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/matches ids ulid #12

Merged

Conversation

Ashk2a
Copy link

@Ashk2a Ashk2a commented Jul 13, 2023

Summary

Add possibility to match ULID route key.

FYI

Copy link
Contributor

@lindyhopchris lindyhopchris left a comment

Choose a reason for hiding this comment

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

Thanks for this PR - looks like a good thing to add! Can you back out the change to the .gitignore? Ignoring IDE folders I'd expect in a global gitignore, not the project one.

Also, can you confirm where you got the regex for the ULID? Just so I can check it before merging and tagging.

.gitignore Outdated Show resolved Hide resolved
@Ashk2a
Copy link
Author

Ashk2a commented Jul 13, 2023

Thanks for this PR - looks like a good thing to add! Can you back out the change to the .gitignore? Ignoring IDE folders I'd expect in a global gitignore, not the project one.

Also, can you confirm where you got the regex for the ULID? Just so I can check it before merging and tagging.

I put information in the description. Like I said I didn't found an "official" regex for the ULID.
ChatGPT give me that : ^[0-9A-HJKMNP-TV-Z]{26}$

https://chat.openai.com/share/e9555822-4ffb-4845-8e40-0bc6cbbc658d

@Ashk2a Ashk2a requested a review from lindyhopchris July 13, 2023 21:19
@lindyhopchris
Copy link
Contributor

@Ashk2a thanks for the info about the regex- sorry I missed that in your original message.

I think we should match what Laravel does for route regexes for ULIDs. Looking at this docs:
https://laravel.com/docs/10.x/routing#parameters-regular-expression-constraints

There's a whereUlid() method when defining routes. That is here:
https://github.com/laravel/framework/blob/53b02b3c1d926c095cccca06883a35a5c6729773/src/Illuminate/Routing/CreatesRegularExpressionRouteConstraints.php#L48

Can you match the regex in your PR to the Laravel one?

Sorry for the back and forth on this - only had time to look at the regex this evening. This is a great addition, so thanks for your time with it - I'm just making sure it's definitely right before merging!

@Ashk2a
Copy link
Author

Ashk2a commented Jul 20, 2023

@Ashk2a thanks for the info about the regex- sorry I missed that in your original message.

I think we should match what Laravel does for route regexes for ULIDs. Looking at this docs: https://laravel.com/docs/10.x/routing#parameters-regular-expression-constraints

There's a whereUlid() method when defining routes. That is here: https://github.com/laravel/framework/blob/53b02b3c1d926c095cccca06883a35a5c6729773/src/Illuminate/Routing/CreatesRegularExpressionRouteConstraints.php#L48

Can you match the regex in your PR to the Laravel one?

Sorry for the back and forth on this - only had time to look at the regex this evening. This is a great addition, so thanks for your time with it - I'm just making sure it's definitely right before merging!

Thanks, very good idea to check Laravel directly!
Change made!

@lindyhopchris lindyhopchris merged commit 5985f66 into laravel-json-api:develop Jul 20, 2023
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.

3 participants