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

- unify locations for normal and endless method definition #718

Merged
merged 1 commit into from Jul 6, 2020

Conversation

@marcandre
Copy link
Contributor

@marcandre marcandre commented Jul 4, 2020

This way one can ask loc.end or loc.assignment of all method definitions

@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Jul 4, 2020

Note: this keeps locations untouched for Module

Copy link
Collaborator

@iliabylich iliabylich left a comment

LGTM, one small note

loc(end_t))
Source::Map::MethodDefinition.new(loc(keyword_t),
loc(operator_t), loc(name_t),
loc(end_t), nil, nil)
end

def endless_definition_map(keyword_t, operator_t, name_t, assignment_t, body_e)

This comment has been minimized.

@iliabylich

iliabylich Jul 5, 2020
Collaborator

let's rename this method to method_definition_map

This comment has been minimized.

@marcandre

marcandre Jul 6, 2020
Author Contributor

Sure. But we'll need to keep an alias for compatibility, right?

This comment has been minimized.

@iliabylich

iliabylich Jul 6, 2020
Collaborator

endless_definition_map is (was) used only in ruby28.y that is still in development. I think it's ok to rename without any aliases

This comment has been minimized.

@marcandre

marcandre Jul 6, 2020
Author Contributor

Oh, sorry, I thought you wanted definition_map => method_definition_map.

Would you like to rename endless_definition_map => method_definition_map and that this is used for standard and endless method definitions?

My PR uses definition_map for standard methods, and endless_definition_map for endless ones. This way definition_map remains as a valid API and the implementation is updated, but maybe there's a better way to go.

This comment has been minimized.

@marcandre

marcandre Jul 6, 2020
Author Contributor

(seems it would be confusing if standard methods used definition_map and endless ones used method_definition_map)

This comment has been minimized.

@iliabylich

iliabylich Jul 6, 2020
Collaborator

nvm, I think it's better to keep it as is.

EDIT: just got a notification from GH that you reverted it. Yes, let's keep it as is, otherwise it sounds like it handles all method definitions. 👍

This comment has been minimized.

@marcandre

marcandre Jul 6, 2020
Author Contributor

Yes, I had misunderstood your comment (although you clearly made it on the def endless_definition_map, I should have paid more attention)

@marcandre marcandre force-pushed the marcandre:endless_loc branch 2 times, most recently from ca95288 to 6826c1a Jul 6, 2020
Copy link
Collaborator

@iliabylich iliabylich left a comment

👍

@iliabylich iliabylich changed the title Unify locations for method and endless method definitions [#716] - unify locations for normal and endless method definition Jul 6, 2020
@iliabylich iliabylich merged commit 07ab97e into whitequark:master Jul 6, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -1856,6 +1856,7 @@ def test_def
%q{def foo; end},
%q{~~~ keyword
| ~~~ name
|! assignment

This comment has been minimized.

@marcandre

marcandre Jul 6, 2020
Author Contributor

@iliabylich I'm glad you suggested we introduce this notation, didn't think I'd use it so soon 🙇‍♂️

@iliabylich
Copy link
Collaborator

@iliabylich iliabylich commented Jul 6, 2020

@marcandre Do you need a release?

@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Jul 6, 2020

Not really, but maybe @palkan would like one (he pointed this out)

@marcandre marcandre deleted the marcandre:endless_loc branch Jul 6, 2020
@palkan
Copy link
Contributor

@palkan palkan commented Jul 7, 2020

No rush, I'm good, thanks) Just want to make sure the tests pass against master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.