-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Add Kernel#Pathname spec #810
Conversation
core/kernel/pathname_spec.rb
Outdated
new_path = Pathname(path) | ||
|
||
(path.object_id == new_path.object_id).should be_true | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but it's common to use equal
matcher to check whether objects are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! thanks
core/kernel/pathname_spec.rb
Outdated
@@ -0,0 +1,26 @@ | |||
require_relative '../../spec_helper' | |||
require 'pathname' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pathname
is a standard library, could you move the spec to library/pathname/Pathname_spec.rb
?
The reason is no standard libraries should be require
-d when running core/
specs.
core/kernel/pathname_spec.rb
Outdated
|
||
describe "Kernel#Pathname" do | ||
it "is a public method" do | ||
Kernel.public_methods.should include(:Pathname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Kernel.should have_private_instance_method(:Pathname)
since the spec is about Kernel#Pathname.
There is also Kernel.Pathname
, which we could test in a separate it
with Kernel.should have_method(:Pathname)
.
@HeroProtagonist Could you address the 2 things I mentioned above? |
d8ee7a6
to
dec7b6e
Compare
@eregon 👋 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
relates to #745