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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

update syn&quote #4

Merged
merged 8 commits into from Jan 8, 2022
Merged

update syn&quote #4

merged 8 commits into from Jan 8, 2022

Conversation

hoodie
Copy link
Contributor

@hoodie hoodie commented Dec 20, 2021

Hi there, this is my work in progress, I just spent the weekend getting up to speed with proc-macros at all.

Full transparency: this doesn't compile at all yet. I don't want to impose, but if you feel like pitching in, it would be much appreachiated, it is rather demanding work making this compile. 馃槈

@hoodie hoodie changed the title WIP WIP: update syn&quote Dec 20, 2021
@koivunej
Copy link
Owner

I haven't been able to end my end of the bargain (fixing CI etc) because of $non_project_issues but great you already got a PR. I'll try to go through it end of today.

@hoodie
Copy link
Contributor Author

hoodie commented Dec 21, 2021

this is absolutely NOT urgent BTW
for me this is basically just the difference between being able to name a field r#type vs typ, that's it.
so no pressure please

@hoodie hoodie force-pushed the feature/update-syn branch 2 times, most recently from c1ffa2e to 0200c58 Compare December 23, 2021 11:09
@hoodie
Copy link
Contributor Author

hoodie commented Dec 23, 2021

So I finally got the lib to compile. but now not all the tests compile anymore because I might have missed some cases.

koivunej added a commit that referenced this pull request Dec 23, 2021
should revert once #4 is merged.
koivunej added a commit that referenced this pull request Dec 23, 2021
should revert once #4 is merged.
@koivunej
Copy link
Owner

Looking good so far! I've migrated this project to github actions so just do a rebase on top of the latest master when you feel like it.

Also noticed that I didn't quite see how should the derive functionality work in the first place :) This currently works on a duck-typing approach (mere existence of val.into_owned() is enough), which I guess I am surprised now that it worked at all. There probably should had been a top level crate for a trait of this conversion, and implementations of it for the core/std types and so on. But we can get to that in the aftermath of this upgrade :)

@hoodie
Copy link
Contributor Author

hoodie commented Dec 23, 2021

As far as I understand proc-macros for now you don't know much about the code other than the syntax itself. So yes, the safest thing seems to be to implement your own trait and just expect/require that all types in the fields of your struct do so too.
Also what I learned from the proc-macro-workshop: it is super fine to just panic for cases you don't want to handle, that panic message will even show up as a compile error, which is quite convenient.

@hoodie hoodie force-pushed the feature/update-syn branch 2 times, most recently from ad38b90 to 2e8be3d Compare December 25, 2021 20:16
@hoodie
Copy link
Contributor Author

hoodie commented Dec 25, 2021

Merry Xmas! I kinda worked on this on and off in between some family time. Sorry about the weird format and all the dbg!() macros still, I very much intend to rebase this into something much clearer eventually.
Right now it already seems to pass all but the enum tests.

@hoodie hoodie changed the title WIP: update syn&quote update syn&quote Dec 28, 2021
@hoodie
Copy link
Contributor Author

hoodie commented Dec 28, 2021

Hi @koivunej. I thing I got it done.
Would be great if you could check it out.
Thanks

Copy link
Owner

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Thank you very much!

I think this is looking good, you know more than I do at this point. One question so far popped in to mind while trying to understand the diffs. Thanks for keeping my type_hopefully_is vagueness :)

As the tests pass I'm inclined to just merge this straight away. You were interested in adding the support for raw identifiers so please go ahead. If you had some parts where you were looking for my input specifically, we can revisit this PR even after merging.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
.iter()
.enumerate()
.filter(|(_, field)| field.ident.is_none())
.map(|(index, _)| format_ident!("{}", abc(index).expect("so many indexes")))
Copy link
Owner

Choose a reason for hiding this comment

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

The previous way for ident generation was x{index} ... The a..=z is probably enough for everyone, just wondering if you picked this up from some material as a suggested way to generate identifiers? I wasn't able to find google this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that's a good idea actually, I was a bit in creative mood at that moment, but x{index} or possibly arg_{index} are actually more reasonable, will do that

src/lib.rs Show resolved Hide resolved
hoodie and others added 2 commits January 6, 2022 19:43
Co-authored-by: Joonas Koivunen <joonas.koivunen@gmail.com>
@hoodie
Copy link
Contributor Author

hoodie commented Jan 8, 2022

In case you鈥榬e waiting on anything regrading raw identifiers, there is nothing left to do. They come for free with the update.

@koivunej
Copy link
Owner

koivunej commented Jan 8, 2022

All right, thanks again @hoodie. I'll merge this and close my hoodie#1, maybe add back that test case and do a bit of fumbling around releasing.

@koivunej koivunej merged commit a756d6d into koivunej:master Jan 8, 2022
@koivunej koivunej mentioned this pull request Jan 8, 2022
@hoodie
Copy link
Contributor Author

hoodie commented Jan 8, 2022

Thanks @koivunej, sorry about hoodie#1, I hadn't seen that issue yet

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

2 participants