Skip to content

Conversation

@jsam
Copy link
Contributor

@jsam jsam commented Dec 11, 2019

closes: #768

@jsam jsam requested a review from a team as a code owner December 11, 2019 18:16

argv = [os.path.basename(sys.argv[0])
] + [remove_credentials(arg) for arg in sys.argv[1:]]
if commit_message is None or not isinstance(commit_message, str):
Copy link
Contributor

Choose a reason for hiding this comment

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

It's good to check for the empty message as well. Also, for the case of non-string message, I'd rather raise an exception or just use str(commit_message) to make it visible instead of silently ignoring it. Since we are the user of this API, str(commit_message) would be enough.

Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

This is going to be a nice improvement, but why limit it to the dataset commands?

@jsam jsam changed the title feat: add optional commit message to dataset commands feat: add optional commit message Dec 12, 2019
@jsam
Copy link
Contributor Author

jsam commented Dec 12, 2019

This is going to be a nice improvement, but why limit it to the dataset commands?

I've added now explicit params to all other commands.

Copy link
Contributor

@mohammad-alisafaee mohammad-alisafaee left a comment

Choose a reason for hiding this comment

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

👍

@jsam jsam merged commit a3116fa into master Dec 12, 2019
@jsam jsam deleted the 768_commit_msg branch December 12, 2019 20:11
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.

RaaS: Commit messages should be specified as optional parameters

4 participants