Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generalizes to support multiple migrators #16
Generalizes to support multiple migrators #16
Changes from 4 commits
cf6d3c8
ac3b394
5a58296
28c09b8
0b6a061
8bd5417
5ac0dc6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned above, it's a code smell when you're directly modifying another class's collection. It's also a code smell when you're making calls to the class that "owns" your class, rather than letting it make calls to you.
I'd probably make this method return the result of
getMigratedContents()
. Then theMigrator
can choose what to do with that information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to have
migrated
as a field ofMigrationVisitor
, with any dependency visitors created withnewInstance
getting a reference to the same map so that they can all add to it and the entrypoint's visitor can pass the complete map back toMigrator
whenrun
returns.Do you think this is fine, or would it be better to have each visitor maintain its own map and then copy the results from each of its dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I'd say make the
migrated
field private toMigrationVisitor
and give it total responsibility for handling the output, but you need to pass it throughnewInstance()
somehow...What if we restructure this so that you don't need to construct multiple visitors at all? Rather than passing a URL to the constructor, pass it to
run()
. That also means that when we get around to de-duplicating repeated imports/uses (which will probably be necessary for large-scale performance), there's a natural place for that as well.This does mean that per-file state can't just be stored as final fields on the subclass. But you can override
visitStylesheet()
to set it temporarily:This is basically how Dart Sass's
EvaluateVisitor
works. WDYT?