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

feat(cli): generate multiple outputs to different files in a single c… #2011

Conversation

albertored
Copy link
Contributor

@albertored albertored commented Dec 23, 2021

Fixes #1970 .

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

@albertored albertored force-pushed the feat/cli/multiple-output-one-call branch 2 times, most recently from 8f94078 to 2471805 Compare December 23, 2021 16:42
Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Hey!
My apologies for such a delayed review on this one.
I'm thinking we could perhaps do output.html, output.junit and so on?
Might cause less friction. What do you think?

@fgreinacher
Copy link
Contributor

fgreinacher commented Jan 19, 2022

I think we could simplify this even more by keeping the existing options with some extra syntax support:

  • --format json,junit,stylish to generate multiple formats.
  • --output json:results.json,junit:results.xml to specify file names
    • when no filename specified for a formatter, it'll write to stdout (like now)
    • fail if there are multiple formatters and only a single filename (e.g. --format json,junit --output results.txt)
    • fail if there is a filename is specified for a formatter that is not active (e.g. --format json --output junit:results.xml)

WDYT @P0lip @albertored?

@P0lip
Copy link
Contributor

P0lip commented Jan 19, 2022

@fgreinacher
That would work too, I like it.
One could also consider the following:

--format json --format junit

However, I'm not certain about the syntax for the output if we went with the above convention.
We'd probably need something as follows

--output:json <file> --output:junit <file>

@albertored
Copy link
Contributor Author

@fgreinacher I also like it, it is similar to the one I originally suggested in the issue but more clear.
Should we wait for some other feedback or can I implement the needed changes?

@fgreinacher
Copy link
Contributor

@fgreinacher I also like it, it is similar to the one I originally suggested in the issue but more clear.
Should we wait for some other feedback or can I implement the needed changes?

I think @P0lip should have the last call on this 😀

@albertored
Copy link
Contributor Author

I think @P0lip should have the last call on this grinning

Yes, sure, I forgot to tag him

@P0lip
Copy link
Contributor

P0lip commented Jan 21, 2022

@albertored go for it if you have spare time! Thanks a lot for putting all the effort into it.
I like that functionality, so as soon as code is ready, we could have it merged in and released

@albertored albertored force-pushed the feat/cli/multiple-output-one-call branch from 2471805 to 7cfae07 Compare January 21, 2022 17:48
@albertored
Copy link
Contributor Author

albertored commented Jan 21, 2022

@P0lip @fgreinacher done, let me know what do you think, the logic for handling those options is quite complex.

I also have a lint error that I'm not able to fix...

/packages/cli/src/commands/lint.ts
   99:17  error    Unsafe assignment of an `any` value  @typescript-eslint/no-unsafe-assignment

expect(writeOutput).nthCalledWith(1, '<formatted output>', undefined);
expect(writeOutput).nthCalledWith(2, '<formatted output>', 'foo.json');
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test error cases here? For example:

  • --format json,junit --output results.txt (output needs to include format)
  • --format json --output junit:results.xml (unselected format in output)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was waiting to have a feedback on the implementation first

@@ -20,8 +20,8 @@ Options:
--version Show version number [boolean]
--help Show help [boolean]
-e, --encoding text encoding to use [string] [choices: "utf8", "ascii", "utf-8", "utf16le", "ucs2", "ucs-2", "base64", "latin1"] [default: "utf8"]
-f, --format formatter to use for outputting results [string] [choices: "json", "stylish", "junit", "html", "text", "teamcity", "pretty"] [default: "stylish"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like that we lost the choices in the help text.

Wondering whether it would be easier (implementation- and usage-wise) to allow multiple --format and --output parameters instead of the providing multiple values within one parameter, e.g. --format stylish --format junit --output junit:junit.xml instead of of --format stylish,junit --output junit:junit.xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I was looking at having --format json junit but yargs does not behave well with array arguments together with positional ones.

Repeating the --format flag is completely doable instead

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also consider https://github.com/yargs/yargs/blob/main/docs/tricks.md#objects to make the implementation even easier (e.g. --output.junit junit.xml)

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, it feels like using objects might be a good idea for outputs.

I'm trying to explore how invasive changing the type of format option would be.
It feels like we'd need to consider it a breaking change if we changed the type of the option, as yargs would need to be instrumented with -- to stop the parsing, or, alternatively, pass format after the positional arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts #2037?

@P0lip
Copy link
Contributor

P0lip commented Feb 3, 2022

Closing this one as we've already merged #2037.
Once again a huge thanks to all of you.

@P0lip P0lip closed this Feb 3, 2022
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.

Generate multiple output formats with one cli call
3 participants