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

Add --force option to gem sources command #3956

Merged
merged 1 commit into from Sep 25, 2020
Merged

Add --force option to gem sources command #3956

merged 1 commit into from Sep 25, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 18, 2020

Allows --force to override use of http URLs and typo checks

Description:

As discussed in #3955 implements a --force option to the sources command

What was the end-user or developer problem that led to this PR?

Discussed in #3955 as a complete solution to scripted use of gem which cannot handle no tty input within ask_yes_no

What is your fix for the problem, implemented in this PR?

Localised fix within commands/sources_command.rb since implementation within user_interaction.rb suggests the creation of user_interaction_options.rb to follow the framework but this would require a larger scale change to centralise the several already implemented --force options within other modules. --force chosen to be consistent with other module usage vs -y.

Also note that --force potentially does different things in other modules so the refactor mentioned is potentially extensive which is why the override is confined to the sources command implementation.

Closes #3955.


Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@deivid-rodriguez
Copy link
Member

A tiny note, don't reference issues in the PR title or commit message, only in the PR body.

Rationale is:

  • For PR titles, we use those to generate the changelog, and they won't get properly autolinked in that context.
  • For commit messages, I prefer git history to be independent of the source tracker, and to force the commit message author to explain what's being fixed independently without resorting to linking to github. Also, when referencing issues in commits and force-pushing, and reference is generated in the reference ticket for every force-push wich I find a bit annoying 😅.

@ghost
Copy link
Author

ghost commented Sep 18, 2020

A tiny note, don't reference issues in the PR title or commit message, only in the PR body.

Rationale is:

  • For PR titles, we use those to generate the changelog, and they won't get properly autolinked in that context.
  • For commit messages, I prefer git history to be independent of the source tracker, and to force the commit message author to explain what's being fixed independently without resorting to linking to github. Also, when referencing issues in commits and force-pushing, and reference is generated in the reference ticket for every force-push wich I find a bit annoying 😅.

Updated commit message as requested 👍

@deivid-rodriguez deivid-rodriguez changed the title (#3955) --force option for gem source commands Add --force option to gem sources command Sep 18, 2020
@simi
Copy link
Member

simi commented Sep 18, 2020

ℹ️ This adds --force just to this one command only. There's chance to make it global flag (like --verbose) and handle this in ask_yes_no globally.

@ghost
Copy link
Author

ghost commented Sep 18, 2020

ℹ️ This adds --force just to this one command only. There's chance to make it global flag (like --verbose) and handle this in ask_yes_no globally.

@simi I had a chat with @deivid-rodriguez about the approach here so here is the full summary of the above 'Resolved' conversation.

  1. ask_yes_no default only works when tty? is false. Changing this means changing behaviours that existing users may have workarounds in. Could be a breaking change.
  2. ask_yes_no option should probably be consistent with --force used in other commands so extending it's usage would require a refactor of all existing commands and a clear understanding of how --force differs for those other commands. I aggree that it is cleanest to implement a user_interface_options.rb, link ask_yes_no to that etc. but this would be a much larger PR and risker a refactor and I also didn't want to mix a large refactor with a small code change.
  3. --force options are consistently currently implemented in the command scripts so it seems in keeping with the project for this commit to do the same pending a larger refactor effort.

@deivid-rodriguez
Copy link
Member

Yes @simi, adding a global --force option or similar makes total sense, but it probably requires some refactoring and a review of all what all current --force options currently do, in order to not create any naming conflicts. I would open a new ticket to track that improvement.

@ghost
Copy link
Author

ghost commented Sep 20, 2020

9 failed checks but all it says on each one is 'This check failed' on the Details. Could this be a build server restart or something?

@simi
Copy link
Member

simi commented Sep 20, 2020

https://github.com/rubygems/rubygems/actions/runs/261103746

This request was automatically failed because there were no enabled runners online to process the request for more than 1 days.

Yes, it seems like CI problem.

@ghost
Copy link
Author

ghost commented Sep 20, 2020

@simi is there anything that will restart these after it fails in this state or do I have to squash on an empty commit to trigger it to run everything again based on a new commit hash?

@simi
Copy link
Member

simi commented Sep 20, 2020

I can't restart, but you can do git commit --amend --no-edit and force push.

@ghost
Copy link
Author

ghost commented Sep 20, 2020

I can't restart, but you can do git commit --amend --no-edit and force push.

Thanks @simi, done :)

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Looks good to me, just noticed a couple of small things.

test/rubygems/test_gem_commands_sources_command.rb Outdated Show resolved Hide resolved
test/rubygems/test_gem_commands_sources_command.rb Outdated Show resolved Hide resolved
Allows --force to override use of http URLs and typo checks
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks!

@deivid-rodriguez deivid-rodriguez merged commit ff86ba4 into rubygems:master Sep 25, 2020
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Add `--force` option to `gem sources` command

(cherry picked from commit ff86ba4)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Add `--force` option to `gem sources` command

(cherry picked from commit ff86ba4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support -y or --force option in ask_yes_no.
5 participants