-
Notifications
You must be signed in to change notification settings - Fork 209
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 to call collection command in child dir #1025
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think it will have unexpected behavior when the directories are different between
rbs_collection.yaml
andGemfile.lock
.For example:
In this case, it's ok when the current directory is
parent
.But it will have unexpected behavior when the current directory is
child
.In
child
,rbs
command findsparent/rbs_collection.yaml
andparent/child/Gemfile.lock
. But theparent/rbs_collection.yaml
is generated withparent/Gemfile.lock
. They're mismatched. Sorbs collection install
command and so on work unexpectedly in thechild
directory.I have two ideas to avoid the problem.
First, search
Gemfile.lock
only from the same directory asrbs_collection.yaml
. For example, gemfile_lock_path will beconfig_path.dirname.join('Gemfile.lock')
.Second, validate the directories. We can check the directories for them are the same. If the directories are different, it raises an error.
I prefer the first idea. Sometimes a child directory has a
Gemfile.lock
, so the validation may be too strict.What do you think?
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.
@pocke Thank you for reviewing.
You are right. In this case we have a problem with the current change.
We often run commands with
$ bundle exec
.If we use
$ bundle exec rbs collection
in the children directory, bundler will refer to the Gemfile in the children directory.In the case of method 1, if the programmer runs
bundle exec rbs collection
in the children directory, the Gemfile that bundler refers to and the Gemfile that rbs refers to will be different. This also seems to create confusion in this case.In the case of method 2, it would be inconvenient because it would be necessary to generate an unnecessary rbs_collection.yaml.
The fact that the repository has separate Gemfiles means that the parent and child are likely to be different scripts or applications. In some cases, the child script may not need type support.
How about referring to rake as my suggestion for method 3?
In rake, if programmer run
$ bundle exec rake
in the child directory of the situation presented, it will print the absolute path of the referenced Rakefile.https://github.com/ruby/rake/blob/73a21161bbd0db02804bbd11606a7529e2a0aaa2/lib/rake/application.rb#L690-L693
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.
In your suggestion, how does rbs command behave? Which files is it load from the same directory or different directories?
If it loads the files from different directories, I think the message doesn't solve the problem.
rbs collection
command generatesrbs_collection.lock.yaml
fromrbs_collection.yaml
andGemfile.lock
.It also updates
rbs_collection.lock.yaml
ifrbs_collection.yaml
and/orGemfile.lock
have some updates. It means ifrbs collection
command refers differentGemfile.lock
, it changesrbs_collection.lock.yaml
unexpectedly.So I think it always uses
rbs_collection.yaml
andGemfile.lock
from the same directory. But I can understand your concern, which rbs uses different Gemfile.lock between the runtime andrbs collection
.Now I also prefer the idea two. As you said the child directory is for another application in most cases. So it will be safer if rbs raises an error for this situation.
By the way, I think displaying the message is a good idea. We should add the message despite the behavior.
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.
@ksss I need this feature for a feature that I'm implementing now. Can I take over this PR?
I think
rbs collection
should behave like the following (it's method 1).rbs_collection.yaml
upward.pwd + 'rbs_collection.yaml'
pwd + '../' + 'rbs_collection.yaml'
Gemfile.lock
from the same directoryrbs_collection_path.dirname + 'Gemfile.lock'
Gemfile.lock
is different fromBundler.default_lockfile
, print a message.I think this problem is very rare.
If the child directory's
Gemfile
doesn't includerbs
gem,bundle exec rbs
command fails. So we don't need to care about it in this case.If the child directory's
Gemfile
includesrbs
gem, it probably also hasrbs_collection.yaml
. So there is no problem.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.
Sorry, This case is no longer necessary for me and I do not currently have the motivation to fix it...
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 for your reply 🙌
I'll work on this problem next week 👍
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.
Sounds great! Thanks!