CLI: Optional sorting for verdi code export
#6345
Conversation
|
Sry, forgot to update the tests. Will do so if this change is desired. |
|
Even if you remove the explicit sorting, the actual order is not going to be the same as those on the registries, right? It would depend on whether the |
That's a good point, which I didn't really consider. I just tested it, and it seems that when calling When changing the ordering of the fields in the |
And perhaps others will find it surprising if the order changes and/or is not alphabetical. In the end though, these are just configuration files that are machine readable. Just thought that this change wouldn't be that impactful especially if the order is still not necessarily going to be uniform anywhere. But I don't really care that much about it going in either, so let's go ahead and add this change. Could you just please add a test for the option. And since this is (for now) just used in |
Yeah, I guess this just boils down to personal preference in the end. But at least having the option will be nice. Eventually, also having Footnotes
|
GeigerJ2
force-pushed
the
code_export_sort
branch
2 times, most recently
from
0647449 to
fc08e1d
Compare
|
Alright, this should be ready for merge. I actually changed the option from |
| @click.option( | ||
| '-u', | ||
| '--unsorted', | ||
| is_flag=True, | ||
| default=True, | ||
| help="Don't alphabetically sort output yaml configuration file.", | ||
| ) |
There was a problem hiding this comment.
Are you sure this works? The default seems to be True so it will always be unsorted and there is no way to change the value. You would be better off to use a switch in this case rather than a flag. I would suggest
| @click.option( | |
| '-u', | |
| '--unsorted', | |
| is_flag=True, | |
| default=True, | |
| help="Don't alphabetically sort output yaml configuration file.", | |
| ) | |
| @click.option( | |
| '--sort/--no-sort', | |
| default=False, | |
| help="Sort the keys of the output YAML.", | |
| ) |
There was a problem hiding this comment.
I just checked and for me it works:
But why should it not work? The default being True means it's unsorted by default, leading to sort_keys=True in the call to yaml.dump() and as it is a flag, using the -u option should turn unsorted to False, meaning sort_keys=False in the call to yaml.dump().
I'd also be fine with your proposed change, though.
There was a problem hiding this comment.
Though, using the --sort/--no-sort option in that case changes the default to being unsorted, which I wanted to avoid as people might be used to the output being sorted by default as that was the previous behavior. And it requires using the longer --sort option for actually getting the sorted file?
There was a problem hiding this comment.
The default being True means it's unsorted by default, leading to sort_keys=True
That's not true though, right? If the default is True, then sort_keys=True as you say, but then the output will be sorted. I think it is kind of confusing to have it defined this way. If unsorted is True than you are sorting 🤔
The downside of this approach is still that it will be difficult to change the default. Imagine that we would want to change the behavior in the future to make the output unsorted by default, then you would either have to change the option name or force users to specify --unsorted if they want the output sorted 😅
The standard practice for these kinds of switches is to use the --option/--no-option switch. This will allow changing the default without having to change the options and it is a lot clearer as to what is happening.
Though, using the --sort/--no-sort option in that case changes the default to being unsorted, which I wanted to avoid as people might be used to the output being sorted by default as that was the previous behavior.
You can still set the default value to be sorted, i.e. do:
@click.option('--sort/--no-sort', is_flag=True, default=True)
def main(sort):There was a problem hiding this comment.
Yeah, you're right. Not sure why it seemed fine, but I just checked again, and the behavior is reverse, as expected from the actual code... 🙃 The advantage of the --option/--no-option switch makes sense, already seeing the confusion that was emerging now on my side. I'll update it with the --sort/--no-sort switch, then it should be ready to go. Thanks!
tests/cmdline/commands/test_code.py
Outdated
| @@ -273,6 +273,27 @@ def test_code_export(run_cli_command, aiida_code_installed, tmp_path, file_regre | |||
| assert isinstance(new_code, InstalledCode) | |||
|
|
|||
|
|
|||
| @pytest.mark.usefixtures('aiida_profile_clean') | |||
There was a problem hiding this comment.
I think you can use parametrization to include the sort/not-sort variations in the existing test_code_export test. Something like
@pytest.mark.usefixtures('aiida_profile_clean')
@pytest.mark.parametrize('sort', (True, False))
def test_code_export(run_cli_command, aiida_code_installed, tmp_path, file_regression, sort):
"""Test export the code setup to str."""
if sort:
options = ['--sort']
else:
options = ['--no-sort']I think the file_regression should be clever enough to create separate reference files for each parametrized test, but that I am not a 100% sure off. Pretty sure there should be a way though.
There was a problem hiding this comment.
Yeah, I created the new function as the unsorted test was failing, as it was comparing it to the sorted reference file. Didn't consider using @pytest.mark.parametrize, though, that could make it work. Will try!
There was a problem hiding this comment.
Alright, tried this, and it works, file_regression is indeed smart enough. Based on my other comment, I'd vouch for the unsorted option, though.
| @click.option( | ||
| '-u', | ||
| '--unsorted', | ||
| is_flag=True, | ||
| default=True, | ||
| help="Don't alphabetically sort output yaml configuration file.", | ||
| ) |
There was a problem hiding this comment.
I just checked and for me it works:
But why should it not work? The default being True means it's unsorted by default, leading to sort_keys=True in the call to yaml.dump() and as it is a flag, using the -u option should turn unsorted to False, meaning sort_keys=False in the call to yaml.dump().
I'd also be fine with your proposed change, though.
tests/cmdline/commands/test_code.py
Outdated
| @@ -273,6 +273,27 @@ def test_code_export(run_cli_command, aiida_code_installed, tmp_path, file_regre | |||
| assert isinstance(new_code, InstalledCode) | |||
|
|
|||
|
|
|||
| @pytest.mark.usefixtures('aiida_profile_clean') | |||
There was a problem hiding this comment.
Yeah, I created the new function as the unsorted test was failing, as it was comparing it to the sorted reference file. Didn't consider using @pytest.mark.parametrize, though, that could make it work. Will try!
| @click.option( | ||
| '-u', | ||
| '--unsorted', | ||
| is_flag=True, | ||
| default=True, | ||
| help="Don't alphabetically sort output yaml configuration file.", | ||
| ) |
There was a problem hiding this comment.
Though, using the --sort/--no-sort option in that case changes the default to being unsorted, which I wanted to avoid as people might be used to the output being sorted by default as that was the previous behavior. And it requires using the longer --sort option for actually getting the sorted file?
When running `verdi code export`, the generated yaml file is automatically sorted. However, the resulting alphabetical ordering is not what is typically found in the code yaml files in the `aiida-code-registry` and `aiida-resource-registry`. This PR turns the sorting off by default, and provides a command-line option to turn on the sorting. If we don't find any other use cases for this flag, I'd also be fine to just turn off the yaml sorting alltogether, as I don't think this is the format that we'd usually want in the yaml file? I found the previous behavior to be a slight annoyance, while setting up some unfamiliar codes by exporting them from an existing archive and modifying the resulting yaml files accordingly.
|
As always, thanks for your help and assistance @sphuber, very much appreciated 🙏 |
When running
verdi code export, the generated yaml file is automatically sorted. However, the resulting alphabetical ordering is not what is typically found in the code yaml files in theaiida-code-registryandaiida-resource-registry. This PR turns the sorting off by default, and provides a command-line option to turn on the sorting.If we don't find any other use cases for this flag, I'd also be fine to just turn off the yaml sorting alltogether, as I don't think this is the format that we'd usually want in the yaml file?
I found the previous behavior to be a slight annoyance, while setting up some unfamiliar codes by exporting them from an existing archive and modifying the resulting yaml files accordingly.