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

Merge ResolveValue and WriteValue traits #310

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JasperDeSutter
Copy link
Contributor

This is a refactor with the goal to have only 1 implementation of Read/Resolve for ast expressions.
In #305 I had to duplicate some Write implementations to support Resolve as well. That's not necessary anymore after this PR.

The last commit reduces the number of lifetimes on the Scope struct so that the resolve code becomes a little nicer to work with. They didn't need to be separate as it doesn't affect public API.

I'm not sure what to name this, so suggestions to improve "WriteOrResolve" are welcome.

@alerque
Copy link
Collaborator

alerque commented May 6, 2024

Rebased on main to update commit messages and so we can see CI results. I haven't yet given this a serious review to consider what the implementations would be of the breaking changes. I also don't think we have a clear roadmap for upcoming releases but so far in the rush of stuff after the safe-harbor release we haven't implemented anything breaking yet, so we should consider whether we want to do a release series with only non-breaking changes before we move forward on any breaking ones.

@alerque
Copy link
Collaborator

alerque commented May 6, 2024

Rebased again this time using cargo clippy --fix on each commit, just because that's low hanging fruit to get our CI tests showing useful results. That actually made the last commit in the original series a noop because clippy fixed the same stuff in the second commit.

Edit: I forgot to add --all-features for clippy, so redoing that last bit.

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