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

Raise if the user supplies a -C param and it doesn’t exist #4047

Closed
wants to merge 1 commit into from

Conversation

alxgsv
Copy link

@alxgsv alxgsv commented Dec 9, 2018

Added config file existence check as discussed in comments of #3735.

At first I tried to move config file fallback logic from config_parse, because it's not really parsing of a config. But then I thought that it would be more straightforward to just check file existence when optparse catches -C: easier to understand the error when it happens, easier to preserve config file fallback, and easier to understand this logic when/if modifying it later.

@Tensho
Copy link
Contributor

Tensho commented Dec 9, 2018

@alxgsv Hi, thanks for your effort 🙇 I guess you have to incorporate the guard check to this place:

%w[config/sidekiq.yml config/sidekiq.yml.erb].each do |filename|
  opts[:config_file] ||= filename if File.exist?(filename)
end

and it would be more appropriate to move the stuff to validate! method. It's a good practice to leave options parser with only typecasting (as a maximum). Please cover the check with a test here.

@alxgsv
Copy link
Author

alxgsv commented Dec 11, 2018

@Tensho thank you! I completely agree that it's a good practice to leave options parser with only typecasting. That's why I decided not to touch code you've mentioned. It's in the parse_options method but has nothing to do with option parsing. And it leaves no chance to observe original command line state at the validation stage (other then creating some additional flags, checks, etc).

So I decided to touch as less code as possible to fix the bug, but as you said it's not ideal solution too. It's better to close this pull request, because I don't think I can fix this bug in other way without refactoring of parse_options, but I have no time for that.

Please ping me if I should open issue for that!

@alxgsv alxgsv closed this Dec 11, 2018
@Tensho
Copy link
Contributor

Tensho commented Dec 12, 2018

@alxgsv @mperham Fixed in #4054, does it make sense to you?

@alxgsv
Copy link
Author

alxgsv commented Dec 12, 2018

@Tensho for me every solution which throws error in this case works :-) Thank you very much!

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.

None yet

2 participants