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

adds Module#const_source_location specs #815

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

moofkit
Copy link
Contributor

@moofkit moofkit commented Nov 4, 2020

Solving #745

Added Module#const_source_location to retrieve the location where a
constant is defined. Feature #10771

Hello, I'm not sure about how detailed this specs should be.
I've just take examples from https://github.com/ruby/spec/blob/master/core/module/const_get_spec.rb
Is it right? Or better to write different examples?

Also add some special cases from docs https://ruby-doc.org/core-2.7.0/Module.html#method-i-const_source_location

@moofkit moofkit changed the title WIP: adds Module#const_source_location specs adds Module#const_source_location specs Nov 6, 2020
@moofkit moofkit marked this pull request as ready for review November 6, 2020 21:03
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

This looks good in general.
Sorry for the late review.

describe "with dynamically assigned constants" do
it "searches a path in the immediate class or module first" do
ConstantSpecs::ClassA::CS_CONST301 = :const301_1
ConstantSpecs::ClassA.const_source_location(:CS_CONST301).should == [__FILE__, 14]
Copy link
Member

Choose a reason for hiding this comment

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

Could you use something like __LINE__ - 1 to avoid hardcoding line numbers?


describe "with statically assigned constants" do
it "searches location path the immediate class or module first" do
ConstantSpecs::ClassA.const_source_location(:CS_CONST10).should == [@constants_fixture_path, 78]
Copy link
Member

Choose a reason for hiding this comment

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

For these cases, it would be better to define some constants in that file like CS_CONST10_LINE = __LINE__ - 1.

@moofkit
Copy link
Contributor Author

moofkit commented Nov 7, 2020

@eregon thanks for review! Your ideas is very reasonable and brilliant! I will implement it as soon as possible

@moofkit
Copy link
Contributor Author

moofkit commented Nov 7, 2020

done! 🏁

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you for the new specs!

@eregon eregon merged commit db97dd7 into ruby:master Nov 12, 2020
@moofkit moofkit deleted the const_source_location branch November 13, 2020 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants