Skip to content
This repository has been archived by the owner on Apr 16, 2022. It is now read-only.

Issue #110 - Updated usage prompt. #136

Merged
merged 7 commits into from
Apr 23, 2018
Merged

Conversation

RazzM13
Copy link
Contributor

@RazzM13 RazzM13 commented Apr 10, 2018

Fixes #110 by updating "--help" to account for the supported commands and to be shown by default in cases where the required arguments are not provided.

…n by default in cases where the required arguments are not provided.
@RazzM13 RazzM13 changed the title Updated usage prompt. Issue #110 - Updated usage prompt. Apr 13, 2018
Copy link
Owner

@martysweet martysweet left a comment

Choose a reason for hiding this comment

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

Just some small changes :)

src/index.ts Outdated
import validatorBaseImport = require('./validator');
import docsBaseImport = require('./docs');
program
.version(version)
Copy link
Owner

Choose a reason for hiding this comment

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

Can all these .something be indented so it's clear?

src/index.ts Outdated

program.parse(process.argv);
function doNoCommand() {
console.error('\nno command given!');
Copy link
Owner

Choose a reason for hiding this comment

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

Lets format this nicely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but I'm not really sure what you're referring to? :)

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, can we have better English on these feedback messages? As you mentioned in the issue

src/index.ts Outdated
if (typeof firstArg === 'undefined') {
console.error('no command given!');
function doNoArgument() {
console.error('\nmissing required argument');
Copy link
Owner

Choose a reason for hiding this comment

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

Lets format this nicely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto.

@martysweet martysweet added this to the v1.6.1 milestone Apr 19, 2018
@RazzM13
Copy link
Contributor Author

RazzM13 commented Apr 22, 2018

Hi @martysweet, I've just noticed a few small considerations that also occur in master:

  1. The -V option reports a version of 0.0.0 because that is what's specified in package.json;
  2. The -p option can't be used to provide --pseudo parameters, as depicted in the usage text, due to the fact that it's currently being assigned to --parameters;
  3. The --guess-parameters option doesn't provide a short form.

Should we treat these defects here, and if so then how? or should I open separate issues?

@martysweet
Copy link
Owner

  1. I don't want to go through the pain of keeping a version number in master, during build it is set to the correct version (depending on the v1.2.3 version git tag). We could attempt to get from git history but it wouldn't strictly be that version (as it might have commits which haven't been pushed to npm yet) - so it's probably best to leave imho.

  2. Woops. We should probably fix that in a backward compatible manner, updating the usage docs as appropriate.

  3. We could add this.

Regarding new issues, has number 2 changed operation since this refactor or has it also been wrong in the docs?

@RazzM13
Copy link
Contributor Author

RazzM13 commented Apr 22, 2018

The second issue was already occuring, as are all the rest. You can clearly see it from:
https://github.com/martysweet/cfn-lint/blob/master/src/index.ts#L34-L35

@martysweet
Copy link
Owner

martysweet commented Apr 23, 2018

Ok, I'm going to merge this into master.

Lets make an issue to fix the clashing -p and in that issue lets also add a shorthand for --guess-parameters

@martysweet martysweet merged commit b296496 into martysweet:master Apr 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants