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 --no-prompt-ignore option #211

Merged
merged 18 commits into from Mar 30, 2023
Merged

Conversation

lilydjwg
Copy link
Contributor

@lilydjwg lilydjwg commented Mar 24, 2023

closes #210

Checklist

  • Admin:
    • No duplicate PRs
  • Integration:
    • Update unit tests
    • Update shell completions
    • Update configuration file
  • Documentation:
    • Update usage function
    • Update mandoc
    • Update readme
    • Update locales
  • Release:
    • Update change log
    • Update DOWNGRADE_VERSION

Description

What do you think about this? Or a better option name?

I'd like to have the maintainers' options before finishing the checklist.

@pbrisbin
Copy link
Member

The only thing I can think of is if we'd want to extend this functionality, for example to allow adding to ignore without prompting. --no-prompt-ignore makes sense to me as "don't prompt to ignore" but it could also be interpreted as "just ignore without prompting". So I wonder if we'd rather have something like --ignore=never|always|prompt with --ignore=prompt being the default.

WDYT?

@atreyasha
Copy link
Member

Thanks @lilydjwg for starting the PR!

Agree with @pbrisbin, makes sense to be explicit and have these three possibilities.

@lilydjwg
Copy link
Contributor Author

I've changed it to be --ignore never|always|prompt, with a space instead of an equal sign because the option parser doesn't support =.

bin/downgrade Outdated Show resolved Hide resolved
@atreyasha
Copy link
Member

atreyasha commented Mar 27, 2023

Tried some high-level tests of a4fa367 and it works fine. I would say we are good to proceed with next steps in the checklist, which I would be glad to help with.

WDYT?

@lilydjwg
Copy link
Contributor Author

Hi, I ended up doing more than adding the options: the zsh completion and zh_CN translations have been updated too. I can make them separate pull requests if you prefer.

I can rebase the commits if you want.

Copy link
Member

@atreyasha atreyasha left a comment

Choose a reason for hiding this comment

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

Wow thanks for tackling almost everything!

Just a few remaining points that I listed in my review, with the tests being the most important remaining bit.

I can rebase the commits if you want.

Yes, a rebase would be great!

completion/downgrade/zsh Outdated Show resolved Hide resolved
locale/downgrade/zh_CN.po Outdated Show resolved Hide resolved
bin/downgrade Outdated Show resolved Hide resolved
lilydjwg and others added 5 commits March 28, 2023 15:03
Now we mock `pacignore` since we already have integration tests set up
for it in `test/pacignore`
FROM `DOWNGRADE_PROMPT_IGNORE` to `DOWNGRADE_IGNORE`
Copy link
Member

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@atreyasha atreyasha merged commit 37cef53 into archlinux-downgrade:main Mar 30, 2023
3 checks passed
@atreyasha atreyasha mentioned this pull request Mar 31, 2023
2 tasks
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.

Option to not ask to add IgnorePkg
3 participants