-
Notifications
You must be signed in to change notification settings - Fork 252
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
JavaScript mutator improvements #1898
JavaScript mutator improvements #1898
Conversation
d8433f5
to
148e3e3
Compare
packages/javascript-mutator/src/mutators/ArrayDeclarationMutator.ts
Outdated
Show resolved
Hide resolved
packages/javascript-mutator/src/mutators/StringLiteralMutator.ts
Outdated
Show resolved
Hide resolved
WIP for now, waiting for PR #1903 to be reviewed and merged before I finish resolving the review comments. @kmdrGroch it was not clear to me whether we need to go back to if statements for all mutators or not. |
@brodybits Thanks. I only meant these 2 I review. Simple short if's are ok, but if you have multiple ones, it isn't very clean. |
- use raw string mutations internally, whenever possible - generate some of the mutations as AST nodes from the Babel types API - use some ternaries - remove some intermediate const declarations no longer needed - remove a couple of internal utility functions no longer needed
I just merged from
This PR is no longer WIP. Thanks again to @kmdrGroch for the review comments, final review would be much appreciated. Thanks to @simondel for merging the other recent PRs. |
Looks good to me. One question /.thing to check: is "lodash.deepClone" dependency required after these changes? It could.be removed from package.json if it's not. |
e59a772
to
d406f30
Compare
I just did a clean rebase and removed |
And it still works. So 1 dependency less 🚀 |
Back to [WIP] again, since it is now expected to have a conflict with PR #1912 and I would like to break the implementation of JavaScriptMutator.mutate() down into smaller parts again (someday). The one thing is that I find it to be a bit of a waste if I would have to keep working around conflicts due to a slow merge cycle. I am also thinking that if we can unify the JavaScript and TypeScript mutators, as discussed in #1893, it could help reduce the efforts needed for enhancements such as PR #1912 and custom external mutators. |
@brodybits, I was thinking, if some of the changes introduced here, could be merged into my PR, especially from this file: And I was also looking at your code, and I think, there might be an issue since you are not a coping object in some cases. It can lead to some problems with references. (But I am not sure). |
@kmdrGroch I would downvote that idea at this point since I think these 2 PRs are really doing different things and should be mostly orthogonal. I think it would be ideal if we could somehow improve the internal interfaces to keep these 2 PRs completely independent. I think a quicker solution would be to merge this PR now so that you can continue without dealing with a conflict in the future. |
I rather only meant this part where you consolidate |
Got it. I would (still) favor getting this one merged first, for the sake of maintaining more linear progress. |
@simondel could you take a look? I guess this PR is ready to be checked and merged |
Hi! Thanks for making these changes :) I'm reviewing this and I'll also do a diff check between a report from the current master branch and your branch to see if anything other than the arrow mutator has changed. |
lodash.clonedeep
from dependencies in packages/javascript-mutator/package.jsonThese improvements should help reduce the amount of consistency between the JavaScript and TypeScript mutators that I raised in #1893.
/cc @kmdrGroch