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

Ruby: introduce mixin field #2478

Merged
merged 8 commits into from
Apr 1, 2020

Conversation

masatake
Copy link
Member

See #2476.

@AmaiKinono, plesae look at 205052c. It is a test case showing how new code dealing with "include" mixin.

You may be interested in c4711e8 ..205052c.
c4711e8 and 7af2181 are preparation for 0912d1e.

In the future we will introduce a function to get a parser field
with a field type as key. That will be exported to parsers.
This change is for avoiding the confliction in the future.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake
Copy link
Member Author

I thought nestingLevel was not needed because we have cork.
I find it is wrong. To manage information which is not suitable to attach to a tagEntryInfo, nestingLevel is still useful.

@masatake
Copy link
Member Author

I didn't implement reference tags for X and Y because I'm not sure whether it is needed or not.

@coveralls
Copy link

coveralls commented Mar 31, 2020

Coverage Status

Coverage increased (+0.009%) to 86.709% when pulling f9a77b5 on masatake:ruby-mixin-include into 71529ec on universal-ctags:master.

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #2478 into master will decrease coverage by 0.13%.
The diff coverage is 94.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2478      +/-   ##
==========================================
- Coverage   86.73%   86.60%   -0.14%     
==========================================
  Files         177      177              
  Lines       36621    36759     +138     
==========================================
+ Hits        31764    31835      +71     
- Misses       4857     4924      +67     
Impacted Files Coverage Δ
main/entry_p.h 100.00% <ø> (ø)
main/nestlevel.c 87.75% <90.90%> (+0.25%) ⬆️
parsers/ruby.c 98.00% <95.00%> (-0.58%) ⬇️
main/entry.c 87.88% <100.00%> (ø)
main/field.c 95.85% <100.00%> (ø)
main/fmt.c 85.62% <100.00%> (ø)
main/writer-ctags.c 98.70% <100.00%> (ø)
main/writer-json.c 93.67% <100.00%> (ø)
readtags-cmd/readtags-cmd.c 44.94% <0.00%> (-26.11%) ⬇️
dsl/qualifier.c 70.23% <0.00%> (+3.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09b7cc5...f9a77b5. Read the comment docs.

@AmaiKinono
Copy link
Member

AmaiKinono commented Apr 1, 2020

I think I've done the right changes, but now I don't know how to push it to this branch. Maybe you need to open some "push access" for me? Not sure if it can be done on a per branch basis.

Edit: Oh I just got it. I can create a PR on your fork :D Is that the right way?

@masatake
Copy link
Member Author

masatake commented Apr 1, 2020

I think I've done the right changes, but now I don't know how to push it to this branch. Maybe you need to open some "push access" for me? Not sure if it can be done on a per branch basis.

Edit: Oh I just got it. I can create a PR on your fork :D Is that the right way?
Merge state

I will merge this one first. Then you can open your pull request to this repo.

Before merging this, I would like to think about following case:

class A
  def hi
     include X
    p "Calling 'hi' in A."
  end
end

@AmaiKinono
Copy link
Member

AmaiKinono commented Apr 1, 2020

The code you given actually doesn't work on my side. A working code should look like:

  • Use instance method:
class A
  def hi
    self.class.prepend X
    p "Calling 'hi' in A."
  end
end

# a = A.new
# a.hi
# => "Calling 'hi' in A."
# a.hi
# => "Calling 'hi' in X."
  • Or use class method:
class A
  def self.hi
    self.include X
  end
end

# a = A.new
# a.hi
# => (undefined method error)
# A.hi
# a.hi
# => "Calling 'hi' in X."

I think we could just not bother with this, because it can actually happen anywhere:

class A
end
a = A.new

# a.hi
# => (undefined method error)
# A.prepend X
# a.hi
# => "Calling 'hi' in X."

@masatake
Copy link
Member Author

masatake commented Apr 1, 2020

@AmaiKinono , thank you for the explanation.

I found a pattern I would like to support:

module X
  def hi
    p "Calling 'hi' in X."
  end
end

class A
  def self.prep
    include X
  end
  def hi
    p "Calling 'hi' in A."
  end

end

a = A.new
A.prep
p A.ancestors

Recognizing include in the singleton method is not hard.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake
Copy link
Member Author

masatake commented Apr 1, 2020

@AmaiKinono, give me an ack if you think it is ready to merge.
Tell me if you think the last commit should be removed.

@AmaiKinono
Copy link
Member

Yes I think this is ready to merge.

About include in a singleton method: I can see this is useful. I like it. Currently I can't think about similar cases (as I said I am new to ruby), which might be better to add now. But maybe we could extend it in the future when these cases actually come up.

@masatake masatake merged commit 6c50aae into universal-ctags:master Apr 1, 2020
@masatake
Copy link
Member Author

masatake commented Apr 1, 2020

@AmaiKinono, thank you for reviewing.

Feel free to ask me questions about the changes here.

About include in a singleton method: I can see this is useful. I like it. Currently I can't think about similar cases (as I said I am new to ruby), which might be better to add now. But maybe we could extend it in the future when these cases actually come up.

I agree with you.

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

Successfully merging this pull request may close these issues.

3 participants