Skip to content
This repository has been archived by the owner on Feb 4, 2025. It is now read-only.

Add builder classes, make them the only parameter to macro methods #115

Merged
merged 13 commits into from
Oct 25, 2024

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented Oct 23, 2024

This is still a WIP, but I wanted to get it out there for an initial review.

  • Static analysis now is clean
  • JsonCodable works, haven't tried the other ones
  • I still need to get all the tests passing (I am sure many fail, haven't actually checked), will address this tomorrow
  • All builder classes still have the same query API, which needs to be evaluated and hopefully restricted per phase in some way.
  • The QueryClass macro was actually breaking the rules. It is no longer allowed to, so I changed a bit how it works.

@jakemac53 jakemac53 requested a review from davidmorgan October 23, 2024 21:07
Copy link
Contributor

@davidmorgan davidmorgan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

pkgs/macro/lib/src/builders.dart Outdated Show resolved Hide resolved
@jakemac53 jakemac53 requested a review from davidmorgan October 24, 2024 18:15
@jakemac53
Copy link
Contributor Author

jakemac53 commented Oct 24, 2024

@davidmorgan I made some substantive changes to fix some of the tests that you should look at - lazy merged maps are now considered equal if their left/right maps are equal, and _keyof and getParents also were modified for lazy merged maps, which was necessary for qualifiedNameOf to function correctly in the case where a query is made by the macro.

@jakemac53
Copy link
Contributor Author

To fix the remaining tests I had to change the expectations, the qualifiedNameOf function now technically works for cycles and re-used nodes (although the result could be surprising especially in the re-used nodes case).

I am not sure if this is a bad thing or a good thing... but these nodes shouldn't be re-used anyways.

@davidmorgan
Copy link
Contributor

Thanks! Still LGTM.

Re: lists and cycles, both seem fine for now.

@davidmorgan davidmorgan merged commit a3ab801 into main Oct 25, 2024
45 checks passed
@davidmorgan davidmorgan deleted the builder-argument branch October 25, 2024 14:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants