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

Rework attribute parsing #290

Merged
merged 10 commits into from
Apr 3, 2024
Merged

Rework attribute parsing #290

merged 10 commits into from
Apr 3, 2024

Conversation

escritorio-gustavo
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo commented Apr 2, 2024

Goal

This PR aims to change the way we parse attributes in a few key ways:

  • Introducing a couple of traits for the methods all attribute types share
  • Change the merge method to remove the mutable reference
    • The reasoning for this is mostly that I think a for_each that mutates external data is somewhat confusing, so I changed it to use fold (think JS Array.protoype.reduce) instead.
  • Bring all the attribute compatibility validation to the same file where parsing is done

Checklist

  • I have followed the steps listed in the Contributing guide.
  • If necessary, I have added documentation related to the changes made.
  • I have added or updated the tests related to the changes made.
    • This is a refactor, so there isn't really new anything I can test... We could make tests for whether or not our compiler errors are correct, but we still have to figure out how to do that

Comment on lines +50 to +53
rename_all: variant_attr.rename_all.or(match variant_fields {
Fields::Named(_) => enum_attr.rename_all_fields,
Fields::Unnamed(_) | Fields::Unit => None,
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I managed to move the rename_all_fields match to a better location and get rid of that duplicated mehtod

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 2, 2024

I'll take a closer look once I get home, but I love what I've seen so far. Great work!

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Great stuff, nothing here I don't like!
As far as I'm concerned, you can merge this as-is - unless there's anything else you'd like to do.

@escritorio-gustavo
Copy link
Contributor Author

Great stuff, nothing here I don't like!

Right as you say that I push a compiler error lmao

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

What was the reasoning behind the last commit? Was there a bug I missed?

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Right as you say that I push a compiler error lmao

😆
Absolutely no pressure, take your time if there's still stuff you'd like to improve! Definetely like the direction in which this is going!

@escritorio-gustavo
Copy link
Contributor Author

What was the reasoning behind the last commit? Was there a bug I missed?

I thought it was weird passing the ts attribute into parse_serde_attrs for the initial value of fold, so i changed it to avoid doing that

@escritorio-gustavo
Copy link
Contributor Author

take your time if there's still stuff you'd like to improve

I think that was the last thing that I wanted to change, as long as you like the latest commit, I think this is ready to merge

@escritorio-gustavo
Copy link
Contributor Author

I think that was the last thing that I wanted to change, as long as you like the latest commit, I think this is ready to merge

Actually, I just realized that the merge_with_serde method will cause a pretty annoying difference, which is that to make a serde attr compatible with a ts attr merge_with_serde needs to be updated, instead of just the impl_parse block

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Yeah, that's true. I too thought passing in the "previous" attrs was a tiny bit akward, but I think I preferred it over having the separate merge_with_serde fns.

@escritorio-gustavo
Copy link
Contributor Author

In ba04528 removing merge_with_serde and using merge broke the merging of docs, so I just moved the parse_docs part to be after the serde attribute parsing

@escritorio-gustavo
Copy link
Contributor Author

Alright, I think I'm happy with this now!

Copy link
Collaborator

@NyxCode NyxCode left a comment

Choose a reason for hiding this comment

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

So am I!
Great work, super happy you tackled this!

@escritorio-gustavo escritorio-gustavo merged commit 40360d4 into main Apr 3, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the rework_attr_parsing branch April 3, 2024 14:08
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.

2 participants