Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quote route parameters #16

Merged
merged 4 commits into from
Nov 12, 2021

Conversation

osdiab
Copy link
Collaborator

@osdiab osdiab commented Nov 11, 2021

Closes #14

Ensures that it always produces valid typescript even if hyphens are in the path name.

@ckastbjerg
Copy link
Owner

Hmm, not sure how to best resolve this one. A bit to sketchy in the Github webeditor.
I don't have experience with merging from forks. Should I do something or should you? :)

...and this snapshot test will quickly become a pain I see 🤦

@ckastbjerg
Copy link
Owner

Also, I'll bump the patch version once this is merged and I have tested a bit on my own project. Still doing releases by hand here ;)

@osdiab
Copy link
Collaborator Author

osdiab commented Nov 12, 2021

I think this is easiest:

  1. Open terminal, check out this branch
  2. merge main into this branch
  3. there will be conflicts on that snapshot file - just rerun yarn test to regenerate the snapshot
  4. make sure the snapshot makes sense
  5. git commit, to complete the merge commit
  6. push that, and you should be good to merge this into main

@osdiab
Copy link
Collaborator Author

osdiab commented Nov 12, 2021

i just did the merge, so should be good now - the snapshot diff looked like this:

Screen Shot 2021-11-12 at 10 10 38

@ckastbjerg
Copy link
Owner

Thanks for fixing it :)

I think this is easiest:

  1. Open terminal, check out this branch
  2. merge main into this branch
  3. there will be conflicts on that snapshot file - just rerun yarn test to regenerate the snapshot
  4. make sure the snapshot makes sense
  5. git commit, to complete the merge commit
  6. push that, and you should be good to merge this into main

Thanks for taking the time to explain! But I do know how to resolve conflicts. The thing was, that I couldn't checkout this branch (osdiab:osdiab/quote-and-escape-routes) as it comes from your fork. Guess I would need access to your fork and resolve them there or have you do it as you did :9

@ckastbjerg ckastbjerg merged commit 75d8f7c into ckastbjerg:main Nov 12, 2021
@osdiab
Copy link
Collaborator Author

osdiab commented Nov 14, 2021

Ah I figured there was some way for you to check it out since when I make the PRs it has a checkbox to allow edits from maintainers - lol maybe there is but idk. As more stuff comes up I'll try to deal with conflicts like that when they come :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creates malformed type if route param contains a dash
2 participants