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.

Sign up for GitHub

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

Jump to bottom

- unify locations for normal and endless method definition #718

Merged
merged 1 commit into from Jul 6, 2020
Merged

- unify locations for normal and endless method definition #718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/parser.rb
View file
Open in desktop
Expand Up @@ -46,7 +46,7 @@ module Source
require 'parser/source/map/variable'
require 'parser/source/map/keyword'
require 'parser/source/map/definition'
require 'parser/source/map/endless_definition'
require 'parser/source/map/method_definition'
require 'parser/source/map/send'
require 'parser/source/map/index'
require 'parser/source/map/condition'
Expand Down
12 changes: 6 additions & 6 deletions lib/parser/builders/default.rb
View file
Open in desktop
Expand Up @@ -1803,17 +1803,17 @@ def module_definition_map(keyword_t, name_e, operator_t, end_t)
end

def definition_map(keyword_t, operator_t, name_t, end_t)
Source::Map::Definition.new(loc(keyword_t),
loc(operator_t), loc(name_t),
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's rename this method to method_definition_map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

body_l = body_e.loc.expression

Source::Map::EndlessDefinition.new(loc(keyword_t),
loc(operator_t), loc(name_t),
loc(assignment_t), body_l)
Source::Map::MethodDefinition.new(loc(keyword_t),
loc(operator_t), loc(name_t), nil,
loc(assignment_t), body_l)
end

def send_map(receiver_e, dot_t, selector_t, begin_t=nil, args=[], end_t=nil)
Expand Down
8 changes: 5 additions & 3 deletions lib/parser/source/map/endless_definition.rb → lib/parser/source/map/method_definition.rb
View file
Open in desktop
Expand Up @@ -3,19 +3,21 @@
module Parser
module Source

class Map::EndlessDefinition < Map
class Map::MethodDefinition < Map
attr_reader :keyword
attr_reader :operator
attr_reader :name
attr_reader :end
attr_reader :assignment

def initialize(keyword_l, operator_l, name_l, assignment_l, body_l)
def initialize(keyword_l, operator_l, name_l, end_l, assignment_l, body_l)
@keyword = keyword_l
@operator = operator_l
@name = name_l
@end = end_l
@assignment = assignment_l

super(@keyword.join(body_l))
super(@keyword.join(end_l || body_l))
end
end

Expand Down
2 changes: 2 additions & 0 deletions test/test_parser.rb
View file
Open in desktop
Expand Up @@ -1856,6 +1856,7 @@ def test_def
%q{def foo; end},
%q{~~~ keyword
| ~~~ name
|! assignment
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

| ~~~ end})

assert_parses(
Expand Down Expand Up @@ -9585,6 +9586,7 @@ def test_endless_method
%q{~~~ keyword
| ~~~ name
| ^ assignment
|! end
|~~~~~~~~~~~~~~ expression},
SINCE_2_8)

Expand Down