-
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
Conversation
@@ -1001,12 +1001,12 @@ def run_collection(args, options) | |||
case args[0] | |||
when 'install' | |||
unless params[:frozen] | |||
Collection::Config.generate_lockfile(config_path: config_path, gemfile_lock_path: Pathname('./Gemfile.lock')) | |||
Collection::Config.generate_lockfile(config_path: config_path, gemfile_lock_path: Bundler.default_lockfile) |
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
and Gemfile.lock
.
For example:
parent/
├── Gemfile
├── Gemfile.lock
├── child
│ ├── Gemfile
│ └── Gemfile.lock
└── rbs_collection.yaml
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 finds parent/rbs_collection.yaml
and parent/child/Gemfile.lock
. But the parent/rbs_collection.yaml
is generated with parent/Gemfile.lock
. They're mismatched. So rbs collection install
command and so on work unexpectedly in the child
directory.
I have two ideas to avoid the problem.
First, search Gemfile.lock
only from the same directory as rbs_collection.yaml
. For example, gemfile_lock_path will be config_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.
$ cd /parent/child
$ bundle exec rake -T
(in /parent)
...
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.
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.
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 generates rbs_collection.lock.yaml
from rbs_collection.yaml
and Gemfile.lock
.
It also updates rbs_collection.lock.yaml
if rbs_collection.yaml
and/or Gemfile.lock
have some updates. It means if rbs collection
command refers different Gemfile.lock
, it changes rbs_collection.lock.yaml
unexpectedly.
So I think it always uses rbs_collection.yaml
and Gemfile.lock
from the same directory. But I can understand your concern, which rbs uses different Gemfile.lock between the runtime and rbs 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).
- Search
rbs_collection.yaml
upward.- Search
pwd + 'rbs_collection.yaml'
- If it doesn't exist, search
pwd + '../' + 'rbs_collection.yaml'
- Do this process until the file is found or the directory is root.
- Search
- Search
Gemfile.lock
from the same directory- It means
rbs_collection_path.dirname + 'Gemfile.lock'
- It means
- (optional) If the config file is not placed in the current directory, print a message like Rake.
- (optional) If the
Gemfile.lock
is different fromBundler.default_lockfile
, print a message.- It is maybe too much because the previous message is also displayed in this case.
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.
I think this problem is very rare.
If the child directory's Gemfile
doesn't include rbs
gem, bundle exec rbs
command fails. So we don't need to care about it in this case.
If the child directory's Gemfile
includes rbs
gem, it probably also has rbs_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!
I am inconvenienced by the specification that I have to go to the root directory every time when I run the rbs collection commands.
I propose that the rbs collection command be able to be executed in the child directory, like bundler or rake.