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 default value set for bool type #273

Merged
merged 8 commits into from
Jul 20, 2023

Conversation

AllyW
Copy link
Contributor

@AllyW AllyW commented Jul 19, 2023

bool is subclass of int, while default True or False should not be converted into int 1 or 0.
Otherwise, type check would fail for aaz cmds (https://github.com/Azure/azure-cli/blob/58ac6ab07fa850e1a6458dec87af91cd6725069e/src/azure-cli-core/azure/cli/core/aaz/_field_type.py#L47)

@AllyW
Copy link
Contributor Author

AllyW commented Jul 19, 2023

in this pr: Azure/azure-cli#26819
aaz cmd:
image

before:
image
after:
image

Comment on lines 127 to 128
elif isinstance(arg_default, bool):
pass
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can simply pass here, as this will make it impossible to distinguish whether the received True is user specified or the default value.

Choose a reason for hiding this comment

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

I think it's useless, because it only added is_default property for str and int, not cover the other types, such as bool, float, timestamp etc.

Copy link

@kairu-ms kairu-ms Jul 19, 2023

Choose a reason for hiding this comment

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

BTW, I also found the following lines has a bug when the arg_default is 0 int value

knack/knack/commands.py

Lines 129 to 131 in e496c95

# update the default
if arg_default:
arg.type.settings['default'] = arg_default

Copy link
Member

@jiasli jiasli Jul 19, 2023

Choose a reason for hiding this comment

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

Could you please elaborate what the bug is?

Copy link
Member

Choose a reason for hiding this comment

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

After reviewing the code, I feel this if arg_default: statement is deliberately added in order to ignore False/0 as default.

For example, for arguments --disable-integrity-monitoring and --disable-integrity-monitoring-autoupgrade of az vm create, their default values are False/0, so they are deliberately omitted from the help message:

> az vm create -h
...
    --disable-integrity-monitoring                      : Disable the default behavior of installing
                                                          guest attestation extension and enabling
                                                          System Assigned Identity for Trusted
                                                          Launch enabled VMs and VMSS.
    --disable-integrity-monitoring-autoupgrade          : Disable auto upgrade of guest attestation
                                                          extension for Trusted Launch enabled VMs
                                                          and VMSS.

knack/commands.py Outdated Show resolved Hide resolved
@jiasli
Copy link
Member

jiasli commented Jul 19, 2023

knack/commands.py Outdated Show resolved Hide resolved
Comment on lines 128 to 132
# adjust here is because:
# 1) bool is subclass of int so isinstance(True, int) cannot distinguish between int and bool
# 2) bool is not extendable according to
# https://stackoverflow.com/questions/2172189/why-i-cant-extend-bool-in-python,
# so bool's is_default is ignored for now
Copy link
Member

Choose a reason for hiding this comment

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

Code comment should usually not focus on adjustment, but the status quo.

We can get the short link for a Stackoverflow post via the "share" button:

image

Suggested change
# adjust here is because:
# 1) bool is subclass of int so isinstance(True, int) cannot distinguish between int and bool
# 2) bool is not extendable according to
# https://stackoverflow.com/questions/2172189/why-i-cant-extend-bool-in-python,
# so bool's is_default is ignored for now
# bool's is_default is ignored for now, because
# 1. bool is a subclass of int, so isinstance(True, int) cannot distinguish between int and bool
# 2. bool is not extendable according to https://stackoverflow.com/a/2172204/2199657

@jiasli
Copy link
Member

jiasli commented Jul 20, 2023

As a side note, Azure/azure-cli#16081 took a different approach. It sets the default value of role to None and defaulting it to Contributor manually in custom.py's code.

@jiasli
Copy link
Member

jiasli commented Jul 20, 2023

The C source code of bool also mentions:

https://github.com/python/cpython/blob/832c37d42a395d4ea45994daffa5e41bd74ac1bb/Objects/boolobject.c#L118

The class bool is a subclass of the class int, and cannot be subclassed.

An alternative is to consider developing our own Bool and implementing special methods such as __bool__, __str__, so that it acts like a real bool, and also bears additional attribute is_default.

@jiasli jiasli merged commit 6049784 into microsoft:dev Jul 20, 2023
@jiasli jiasli mentioned this pull request Jul 24, 2023
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.

5 participants