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

Update to syn 2 #185

Merged
merged 5 commits into from
Apr 7, 2024
Merged

Update to syn 2 #185

merged 5 commits into from
Apr 7, 2024

Conversation

omid
Copy link
Contributor

@omid omid commented Feb 29, 2024

🚧 🚧 🚧
It's a breaking change!
🚧 🚧 🚧

Because syn throws an error for keywords, we have to rename type to something else. A general pattern is to use ty instead of type.

@omid
Copy link
Contributor Author

omid commented Mar 7, 2024

@jaemk Reminder :D

@BaxHugh
Copy link
Contributor

BaxHugh commented Apr 4, 2024

Hey @omid, nice work. Looking at it, it mostly just seems like type -> ty. I like that you've made the change in places other than just the macro call for consistency.

Only thing I've noticed is that we've now got a few calls of &String.as_str() which I think are unnecessary so I've removed them in this PR omid#1 to your branch, eveything still works.

It doesn't look like they are needed
@omid
Copy link
Contributor Author

omid commented Apr 4, 2024

Thanks a lot @BaxHugh. Just merged 🙏🏼

@jaemk
Copy link
Owner

jaemk commented Apr 7, 2024

Thanks @omid and @BaxHugh !

@jaemk jaemk enabled auto-merge April 7, 2024 01:47
@jaemk jaemk merged commit fca60ec into jaemk:master Apr 7, 2024
1 check passed
@jaemk
Copy link
Owner

jaemk commented Apr 7, 2024

FYI, I'm going to wait to release these changes since there's a couple more breaking changes in PR

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