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

Shortened CLI option names and updated corresponding tests #946

Merged
merged 20 commits into from
Jan 22, 2024

Conversation

Ahmad-Wahid
Copy link
Contributor

Within this pull request, I've reduced the length of the CLI option names and modified the associated tests accordingly. For instance, --sensor-id has been changed to --sensor, --account-id is now --account, and --user-id has been updated to --user, etc.

Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
@Ahmad-Wahid Ahmad-Wahid self-assigned this Dec 27, 2023
@Ahmad-Wahid Ahmad-Wahid linked an issue Dec 27, 2023 that may be closed by this pull request
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
@Ahmad-Wahid Ahmad-Wahid added this to the 0.19.0 milestone Dec 27, 2023
@Flix6x
Copy link
Contributor

Flix6x commented Dec 27, 2023

Don't forget the Issue comment about nicely deprecating the old CLI options. That means they should keep working like before, while giving a deprecating notice.

Furthermore, the new CLI option should use available schemas for sensor IDs and source IDs.

Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

I'm guessing parts of the documentation should be updated, too.

@Ahmad-Wahid
Copy link
Contributor Author

okay.

Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
@Ahmad-Wahid
Copy link
Contributor Author

Should I add these changes to the cli changelog?

Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
@Flix6x
Copy link
Contributor

Flix6x commented Jan 3, 2024

Should I add these changes to the cli changelog?

Yes, and also to the main changelog.

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Also adapt the small example in documentation/index.rst please.

Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Copy link
Contributor

@victorgarcia98 victorgarcia98 left a comment

Choose a reason for hiding this comment

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

Thanks!

I found some leftover sensor-id references in the documentation:

Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
@Ahmad-Wahid
Copy link
Contributor Author

Also adapt the small example in documentation/index.rst please.

After the change in documentation/index.rst can be considered as an example.

Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

The parameter alias deprecation works nicely. I have two small improvement suggestions, and would like to see proper attribution on SO content.

flexmeasures/cli/data_show.py Show resolved Hide resolved
flexmeasures/cli/utils.py Show resolved Hide resolved
flexmeasures/cli/utils.py Outdated Show resolved Hide resolved
…-names

# Conflicts:
#	documentation/cli/change_log.rst
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

🎉

flexmeasures/cli/utils.py Outdated Show resolved Hide resolved
Signed-off-by: Ahmad Wahid <ahmedwahid16101@gmail.com>
@Ahmad-Wahid Ahmad-Wahid merged commit 8289c12 into main Jan 22, 2024
7 of 9 checks passed
@Ahmad-Wahid Ahmad-Wahid deleted the 823-shorten-cli-field-names branch January 22, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shorten CLI field names
4 participants