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

Common options / parseCommon fix #66

Merged
merged 3 commits into from
Aug 22, 2019
Merged

Conversation

Kexkey
Copy link
Contributor

@Kexkey Kexkey commented Aug 19, 2019

The "common" options verbose, whitelist and no-default-whitelist were ignored. I changed the commander stuff so they are now taken into account.

Will probably solve issue #42 also.

@@ -126,6 +129,7 @@ const upgradeCommand = program
}
options = parseCommon(options)
options.calendars = options.calendar
options = parseCommon(options)
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think this is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed not needed, too much copy-paste :)

@lvaccaro
Copy link
Member

lvaccaro commented Aug 22, 2019

ack with nit 98fbd12

Great! this resolve the regression due to npm commander update done in 114db95 .

@Kexkey
Copy link
Contributor Author

Kexkey commented Aug 22, 2019

re 98fbd12 : The whitelist stuff is not needed for info and stamp but the --verbose option might be useful?

@lvaccaro
Copy link
Member

I'd like to add parseCommon() before both info & stamp.

@Kexkey
Copy link
Contributor Author

Kexkey commented Aug 22, 2019

Cool! It's there. All good, thanks for the review @lvaccaro.

@lvaccaro
Copy link
Member

ack 3eb2f4d

@RCasatta RCasatta merged commit 543bca1 into opentimestamps:master Aug 22, 2019
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.

3 participants