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

parse dots everywhere #20540

Closed
wants to merge 1 commit into from
Closed

parse dots everywhere #20540

wants to merge 1 commit into from

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Feb 9, 2017

This pull request makes op. parse like .op, enabling backwards compatibility (as pointed out in #20249 (comment)) in case we revisit the dot position consistency question. Best!

@ararslan ararslan added needs tests Unit tests are required for this change parser Language parsing and surface syntax labels Feb 9, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Feb 14, 2017

Rebased and now passing CI. Thoughts on how best to test this? (Beyond tossing an instance or two in test/parse.jl, or is that what you had in mind?) Best!

@tkelman
Copy link
Contributor

tkelman commented Feb 14, 2017

hm, looks like this could be breaking for floating point literals without leading 0s?

@Sacha0
Copy link
Member Author

Sacha0 commented Feb 14, 2017

hm, looks like this could be breaking for floating point literals without leading 0s?

Yes: op.s inherit .ops' behavior, i.e. dots bind more tightly to operators than adjacent numeric literals. Thoughts? Thanks!

@stevengj
Copy link
Member

stevengj commented Feb 14, 2017

I don't think that implementing op. as a synonym for .op is the right thing to do unless we decide that we want to get rid of the latter; we shouldn't have both. Since we haven't decided to get rid of .op, this PR seems premature.

I think we should probably parse op.(...) as (op).(...), however — I don't see any need to require parentheses around op in this case.

@stevengj stevengj added the breaking This change will break code label Feb 14, 2017
@stevengj
Copy link
Member

stevengj commented Feb 14, 2017

This PR is breaking because it changes the parsing of 1+.2 etc, as @tkelman pointed out.

@Sacha0
Copy link
Member Author

Sacha0 commented Feb 14, 2017

I don't think that implementing op. as a synonym for .op is the right thing to do unless we decide that we want to get rid of the latter; we shouldn't have both.

Agreed.

Since we haven't decided to get rid of .op, this PR seems premature.

This pull request does not implement op. as a synonym for .op; it merely makes op. parse, addressing (in the case we revisit this issue next dev cycle) your concern in #20249 (comment):

it would take at least two release cycles, because +. is not even parsed as an operator yet so it would be impossible to implement backwards compatibility if you switched in the same release that implemented the parsing

Best!

@stevengj
Copy link
Member

The problem is that even just making op. parse is a breaking change. I don't think we should undertake a breaking change just on the chance that we decide to rename .op to op. in the future.

@Sacha0
Copy link
Member Author

Sacha0 commented Feb 14, 2017

(Ref. #19089)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code needs tests Unit tests are required for this change parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants