Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Transformation setup clean-up - Part I#312

Merged
bettinaheim merged 22 commits intofeatures/transformationSetupfrom
beheim/transformationSetup
Feb 13, 2020
Merged

Transformation setup clean-up - Part I#312
bettinaheim merged 22 commits intofeatures/transformationSetupfrom
beheim/transformationSetup

Conversation

@bettinaheim
Copy link
Contributor

@bettinaheim bettinaheim commented Feb 12, 2020

I added a revised setup for the transformations provided in the Core project. The idea is to build all transformations on top of this setup, which in particular eliminates the C# wrapper in the transformation project.
A syntax tree transformation is structured into the same subtransformations as now, but the shared internal state is now handled much cleaner, eliminating the need for the rather cumbersome type parametrizations on construction. Right now the actual transformation on individual nodes is still the same, with the idea to revise that slightly in a subsequent step.
What is also still missing is a neat way to disable certain subtransformations.
So far I've migrated the transformations in SearchAndReplace, and I'll see that I can migrate some more.

@bettinaheim bettinaheim changed the title Beheim/transformation setup Transformation setup clean-up - Part I Feb 12, 2020
@bettinaheim bettinaheim changed the base branch from master to features/transformationSetup February 12, 2020 16:45
@bettinaheim bettinaheim marked this pull request as ready for review February 12, 2020 16:45
Copy link
Contributor

@alan-geller alan-geller left a comment

Choose a reason for hiding this comment

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

I agree, this is much cleaner. The one comment I have is about naming -- having QsSyntaxTreeTransformation, but just NamespaceTransformation, StatementTransformation, etc., is a bit hard to remember. I assume this is because you have the existing SyntaxTreeTransformation that is really a NamespaceTransformation; while it will be a breaking change, I suggest renaming the existing SyntaxTreeTransformation class as part of this PR.
It also might be nice to have QsSyntaxTreeTransformation have a Transform method that takes a SyntaxTree as input and returns a new SyntaxTree as output...

Copy link
Contributor

@ScottCarda-MS ScottCarda-MS left a comment

Choose a reason for hiding this comment

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

Please remove trailing whitespace in the files added/modified. A regular expression search/replace can make it quick and painless.

@cgranade
Copy link
Contributor

Please remove trailing whitespace in the files added/modified. A regular expression search/replace can make it quick and painless.

I'm not sure about VS2019, but there's a "Trim Trailing Whitespace" command in the VS Code palette that's pretty handy for that.

@bettinaheim
Copy link
Contributor Author

bettinaheim commented Feb 13, 2020

I agree, this is much cleaner. The one comment I have is about naming -- having QsSyntaxTreeTransformation, but just NamespaceTransformation, StatementTransformation, etc., is a bit hard to remember. I assume this is because you have the existing SyntaxTreeTransformation that is really a NamespaceTransformation; while it will be a breaking change, I suggest renaming the existing SyntaxTreeTransformation class as part of this PR.
It also might be nice to have QsSyntaxTreeTransformation have a Transform method that takes a SyntaxTree as input and returns a new SyntaxTree as output...

Yes, I agree on all of that, I just haven't gotten around to cleaning it all up - hence the PR against a feature branch opposed to into master. Right now this also mostly strictly adds code, but the ultimate goal is to replace the old setup, and remove a good junk of code that was needed to deal with internal state in the old setup. This refactoring should thus result in an overall negative code contribution (reducing the amount of source code). Once we've migrated all the transformation we can go ahead and remove the code that is no longer needed, and clean up the subtransformations (there is still no separation between the recursion/walker logic and the actual action/transformation on the node itself). Going over everything and renaming as appropriate seems like a good thing to do as a final step before merging to master.

@bettinaheim
Copy link
Contributor Author

Please remove trailing whitespace in the files added/modified. A regular expression search/replace can make it quick and painless.

I'm not sure about VS2019, but there's a "Trim Trailing Whitespace" command in the VS Code palette that's pretty handy for that.

Thanks, @cgranade!

@bettinaheim bettinaheim merged commit 0288234 into features/transformationSetup Feb 13, 2020
@bettinaheim bettinaheim deleted the beheim/transformationSetup branch February 13, 2020 02:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants