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

Auto implicit fixes #922

Merged
merged 2 commits into from Aug 20, 2017

Conversation

Projects
None yet
2 participants
@FSMaxB
Collaborator

FSMaxB commented Aug 13, 2017

Continuing #921

FSMaxB added some commits Aug 13, 2017

OperatorNode: Fix implicit multiplication
Fixes implicit multiplication when parenthesis is 'auto' and operands
are ConstantNodes.

This is handled by detecting that case and printing parentheses for
ParenthesisNodes even though they normally wouldn't with parenthesis
set to 'auto'.
@josdejong

This comment has been minimized.

Owner

josdejong commented Aug 14, 2017

Thanks Max, I will review your code at the end of this week. Looks good at first sight.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Aug 14, 2017

You should probably review both commits separately, since the first one mostly changes indentation, thereby bloating the diff.

@josdejong

This comment has been minimized.

Owner

josdejong commented Aug 14, 2017

ok makes sense

@josdejong

This comment has been minimized.

Owner

josdejong commented Aug 19, 2017

Your code looks good Max, this is indeed the behavior we're looking for. Thanks

There is one thing in the code that I don't fully understand, function calculateNecessaryParentheses now has a new argument implicit, but as far as I can see this argument is not used in the function?

//handles an edge case of 'auto' parentheses with implicit multiplication of ConstantNode
//In that case print parentheses for ParenthesisNodes even though they normally wouldn't be
//printed.
if ((args.length >= 2) && (root.getIdentifier() === 'OperatorNode:multiply') && root.implicit && (parenthesis === 'auto') && (implicit === 'hide')) {

This comment has been minimized.

@FSMaxB

FSMaxB Aug 19, 2017

Collaborator

@josdejong: The new parameter is used here!

root.implicit says that the current multiplication is implicit. implicit is the option that has been passed in. Either hide or show.

This comment has been minimized.

@josdejong

josdejong Aug 20, 2017

Owner

ahhh, I simply overlooked it.

@josdejong josdejong merged commit 0b846a3 into develop Aug 20, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No new vulnerabilities
Details

@josdejong josdejong deleted the auto-implicit-fixes branch Sep 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment