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

Support constants defined within arrays or hashes #63

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@owst
Contributor

owst commented May 11, 2017

Our code base includes examples such as:

STATUSES = [
  OPEN = 'open',
  ...
]

DISPLAY_MAPPING = {
  CANCELLED = 'cancelled' => 'Cancelled by user',
  ...
}

currently, OPEN and CANCELLED are not picked up by ripper-tags.

This PR aims to support such nested definitions.

I'm not 100% certain of the implications of making on_args now process its args when it was ignoring them before, but none of the existing tests fail. Please let me know what you think!

@mislav

Thanks for your contribution and sorry for the late review!

D = {
E = 3 => F = 4,
x: G = 5

This comment has been minimized.

@mislav

mislav Sep 4, 2017

Collaborator

Do we need to support F and G here, i.e. parsing assignment on the right side of the => hash syntax? Does anyone define constants that way?

This comment has been minimized.

@owst

owst Sep 5, 2017

Contributor

I don't think so, but it falls out "for free" from the use of on_assoc_new (and it would actually be harder to support nested hashes if we tried to only handle definitions in keys)

This comment has been minimized.

@mislav

mislav Sep 5, 2017

Collaborator

but it falls out "for free"

Even though we get is basically for "free", that is not reason enough to test for and support a feature in the library. If we don't need a specific feature, let's not test for it. Keep in mind that every test that we add here, I have to maintain for months/years over future releases of Ruby and ripper-tags itself. Let's keep the scope of the feature focused on just what was the initial goal.

@@ -320,6 +324,8 @@ def on_assign(name, rhs, line, *junk)
namespace = namespace + parts
end
process(rhs)

This comment has been minimized.

@mislav

mislav Sep 4, 2017

Collaborator

Why was this process added?

This comment has been minimized.

@owst

owst Sep 5, 2017

Contributor

If we don't add this process, the Visitor won't discover potential tags on the RHS of a constant assignment. If I remove this call my added test case fails with:

Failure: test_nested_constant_definitions(TagRipperTest)
/Users/owenstephens/code/ripper-tags/test/test_ripper_tags.rb:92:in `test_nested_constant_definitions'
     89:       }
     90:     EOC
     91:
  => 92:     assert_equal ('A'..'G').to_a, tags.map { |t| t[:name] }.sort
     93:     tags.each do |t|
     94:       assert_equal t[:name], t[:full_name]
     95:     end
<["A", "B", "C", "D", "E", "F", "G"]> expected but was
<["A", "D"]>

as the visitor is no longer looking "inside" the expressions in the (top-level) assignments

This comment has been minimized.

@mislav

mislav Sep 5, 2017

Collaborator

Thanks for the explanation. Are we interested in processing RHS at all?

This comment has been minimized.

@owst

owst Sep 5, 2017

Contributor

I'm not sure I understand the question.

We are interested in processing the RHS since we wouldn't otherwise discover the constant definitions on the RHS of the top-level assignment, which was the aim of this PR. Indeed, if we don't process the RHS, in my original example we only get tags for STATUSES and DISPLAY_MAPPING; with the process(rhs) call we also get tags for OPEN and CANCELLED as desired.

@@ -382,12 +388,15 @@ def on_class_eval(name, body)
@namespace.pop
end
def on_args(*args)
process(args)

This comment has been minimized.

@mislav

mislav Sep 4, 2017

Collaborator

I'm worried about the potential negative implications of this. Have you tried running ripper-tags from this branch against a large Ruby codebase such as Rails?

This comment has been minimized.

@owst

owst Sep 5, 2017

Contributor

Yes, great idea. After my subsequent changes, I've confirmed that the tags files have no changes between master and this branch.

owst added some commits Sep 4, 2017

Do not process unary nodes
To avoid a crash when running against the rails codebase
@@ -65,6 +65,9 @@ def on_const_path_ref(a, b)
def on_binary(*args)
end
def on_unary(*args)
end

This comment has been minimized.

@owst

owst Sep 5, 2017

Contributor

This was needed to avoid trying to process a unary method (-@) in a hash in the rails codebase (triggered since we now expose the keys/values of a hash assignment in the sexp)

if args && args[0] == :args
args[1..-1]
end
end

This comment has been minimized.

@owst

owst Sep 5, 2017

Contributor

This is the better way of handling arrays, rather than the on_args processing I first suggested

@owst

This comment has been minimized.

Contributor

owst commented Sep 5, 2017

Thanks for the review @mislav and good suggestion of testing against the rails codebase! Let me know what you think of the changes and I can squash the commits if you're happy.

@mislav

Thanks for working on this and on your patience.

L = {
'some key' => {
M = 2 => 3

This comment has been minimized.

@mislav

mislav Sep 5, 2017

Collaborator

This test is getting too evolved. It represents features that I'm not keen on supporting, even if it's technically trivial at the moment.

Could we simply make a test that verifies only features that we're interested in here? That would be your initial use case:

STATUSES = [
  OPEN = 'open',
]

DISPLAY_MAPPING = {
  CANCELLED = 'cancelled' => 'Cancelled by user',
}
@@ -320,6 +324,8 @@ def on_assign(name, rhs, line, *junk)
namespace = namespace + parts
end
process(rhs)

This comment has been minimized.

@mislav

mislav Sep 5, 2017

Collaborator

Thanks for the explanation. Are we interested in processing RHS at all?

D = {
E = 3 => F = 4,
x: G = 5

This comment has been minimized.

@mislav

mislav Sep 5, 2017

Collaborator

but it falls out "for free"

Even though we get is basically for "free", that is not reason enough to test for and support a feature in the library. If we don't need a specific feature, let's not test for it. Keep in mind that every test that we add here, I have to maintain for months/years over future releases of Ruby and ripper-tags itself. Let's keep the scope of the feature focused on just what was the initial goal.

owst added some commits Sep 5, 2017

@owst

This comment has been minimized.

Contributor

owst commented Sep 5, 2017

I'm slightly stumped by the build failures, it seems that we're getting an additional assoc node in the sexp, but I can't reproduce it locally on Linux or Mac, on a variety of Ruby versions.

In travis the sexp of my test is:

[[:assign, "STATUSES", [[:assign, "OPEN", nil, 2]], 1], [:assign, "DISPLAY_MAPPING", [[:assoc, [:assign, "CANCELLED", nil, 6], nil]], 5]]

but locally it is

[[:assign, "STATUSES", [[:assign, "OPEN", nil, 2]], 1], [:assign, "DISPLAY_MAPPING", [[:assign, "CANCELLED", nil, 6]], 5]]

(There's an additional [:assoc ... ] wrapping the CANCELLED node.)

I could probably add a Visitor#on_assoc method that does the right thing, but I'm hesitant to do so without being to reproduce/explain the issue locally.

@mislav any thoughts or advice? :-)

@mislav

This comment has been minimized.

Collaborator

mislav commented Sep 5, 2017

I could probably add a Visitor#on_assoc method that does the right thing, but I'm hesitant to do so without being to reproduce/explain the issue locally.

I can't reproduce locally either. I'm stumped. I guess we just have to work around this.

@owst owst force-pushed the owst:constants_inside_arrays_and_hashes branch from 89ef46f to 8b254a7 Sep 5, 2017

@owst owst force-pushed the owst:constants_inside_arrays_and_hashes branch from 8b254a7 to 181cf52 Sep 5, 2017

owst added some commits Sep 5, 2017

@owst

This comment has been minimized.

Contributor

owst commented Sep 5, 2017

Ok, I understand what's happened - the Travis build runs off the git fetch origin +refs/pull/63/merge ref (which is the implicit merge commit of my branch into master), which therefore has picked up this commit which was merged to master after my branch.

That commit adds the on_assoc_new method which adds the troubling :assoc sexp entry. I'll update my branch to handle it.

@owst

This comment has been minimized.

Contributor

owst commented Sep 11, 2017

Hi @mislav is there anything else for me to do in order for this to be merged, or can I squash it down to 1 commit ready for a merge?

@mislav

This comment has been minimized.

Collaborator

mislav commented Sep 18, 2017

Just got back from vacation. This PR looks great; thanks for your patience! There's nothing I need you to do right now.

@mislav

mislav approved these changes Sep 18, 2017

@owst

This comment has been minimized.

Contributor

owst commented Sep 18, 2017

Great, thanks @mislav - shall I squash this into one commit? 16 (including debug) commits looks a bit messy for a +40,-1 PR, but I don't know the project's stance on rewriting... I suppose you could also squash it when you merge the PR.

@mislav

This comment has been minimized.

Collaborator

mislav commented Sep 18, 2017

Yes it's easy for me to squash when I merge.

@owst

This comment has been minimized.

Contributor

owst commented Sep 18, 2017

Great, thanks! 👍

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