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

feat: add mkcalendar & Report to body methods in next #5444

Closed

Conversation

keyservlad
Copy link
Contributor

Checklist

For context see #5439

Comment on lines 28 to 30
'SEARCH',
'MKCALENDAR',
'REPORT'
Copy link
Member

@gurgunday gurgunday May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is normally in alphabetical order

Also just realized 'MKCOL' should be above 'MOVE' for the same reason, would be great if you could move it too

@gurgunday
Copy link
Member

The rest of the changes we can pull from main I think, but maybe we should add their shorthands here as well

Like this: #4409

@keyservlad
Copy link
Contributor Author

keyservlad commented May 3, 2024

I merged main and added the route shorthand methods.
Still getting the route-shorthand test failing tho ahah ;D

Edit: Fixed it

@gurgunday
Copy link
Member

gurgunday commented May 3, 2024

yeah looks great, just one issue, because we use squash merge, I think we should merge main to next first, and merge this after that, otherwise all these merge changes will end up being conflicts

edit: or maybe not

Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day>
gurgunday
gurgunday previously approved these changes May 4, 2024
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Oops, wrong place 😄

So we had to manually merge to next and had to include the changes of this PR, thanks anyway for the PR!

@keyservlad
Copy link
Contributor Author

@gurgunday Thank you guys very much for your work. I noticed you didn't include the route shorthand changes for the new methods. If you ever want to implement them, you can find the changes needed in this PR.

Here are the files to change :
docs/Reference/Routes.md

test/types/route.test-d.ts
instance.d.ts

@gurgunday
Copy link
Member

Good spot, the shorthands were added, but you are right that the types weren't, opening the PR again to not forget

@gurgunday gurgunday reopened this May 6, 2024
@keyservlad
Copy link
Contributor Author

I will try to rebase and get rid of the commits that come from the merge if that's ok with you

@gurgunday
Copy link
Member

Yeah but I’d say let’s wait for the merge to main of v5 first, like @jsumners said, or maybe he’ll add it manually

@mcollina mcollina deleted the branch fastify:next May 7, 2024 13:07
@mcollina mcollina closed this May 7, 2024
@gurgunday
Copy link
Member

Again, sorry for the trouble, can you open the type PR again? It can be a new PR to make things easier, and it should target main now

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.

None yet

3 participants