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

fix(chore): use a callable for defaults #559

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

lecrepont01
Copy link
Contributor

Without a callable the defaults are evaluated on import which causes unexpected errors with other commands.

Copy link
Contributor

mergify bot commented Nov 28, 2024

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Changelog requirements

Wonderful, this rule succeeded.
  • any of:
    • -title ~= ^feat
    • label = need changelog
    • label = skip changelog

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?:

🟢 🔎 Reviews

Wonderful, this rule succeeded.
  • #changes-requested-reviews-by = 0
  • #review-requested = 0
  • #review-threads-unresolved = 0

@mergify mergify bot requested a review from a team November 28, 2024 14:05
sileht
sileht previously requested changes Nov 28, 2024
Copy link
Member

@sileht sileht left a comment

Choose a reason for hiding this comment

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

Lazy loading them is a good idea.

We cannot just use a lambda otherwise cli arguments will be ignored and we will always load the default.

@sileht
Copy link
Member

sileht commented Nov 28, 2024

Lazy loading them is a good idea.

We cannot just use a lambda otherwise cli arguments will be ignored and we will always load the default.

I remake a couple of tests and it seems, the issue occurs only when is_flag=True

@sileht
Copy link
Member

sileht commented Nov 28, 2024

I reproduce the issue in a simplier app https://github.com/sileht/click-bug :

# without lambda:
$ poetry run python3 main.py work
False
$ poetry run python3 main.py work -v
True

# with lambda:
$ poetry run python3 main.py buggy
False
$ poetry run python3 main.py buggy -v
False

@lecrepont01
Copy link
Contributor Author

I reproduce the issue in a simplier app https://github.com/sileht/click-bug :

# without lambda:
$ poetry run python3 main.py work
False
$ poetry run python3 main.py work -v
True

# with lambda:
$ poetry run python3 main.py buggy
False
$ poetry run python3 main.py buggy -v
False

ThanksI am not sure I understand why user input does not always prevail when it is passed but I'll take a better look to find a fix.

@lecrepont01 lecrepont01 force-pushed the fix-default-with-callable branch from 9e08c11 to 3f71a07 Compare November 28, 2024 16:53
@mergify mergify bot dismissed sileht’s stale review November 28, 2024 16:53

Pull request has been modified.

@mergify mergify bot requested a review from a team November 28, 2024 16:54
mergify_cli/stack/cli.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team November 29, 2024 07:54
Without callables the defaults are evaluated on import which causes unexpected errors with other commands.
This lazy load the defaults.
mergify bot added a commit that referenced this pull request Nov 29, 2024
@mergify mergify bot added the queued label Nov 29, 2024
@mergify mergify bot merged commit 33b0a0a into Mergifyio:main Nov 29, 2024
5 checks passed
@mergify mergify bot removed the queued label Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants