eask analyze should exit with 1 on errors

[joshbax189]

This moves the test for eask analyze from #275 and also matches the command's behavior to what was proposed in the tests.

Changes

Exit codes

• eask analyze exits with 1 if there are any errors in any of the files checked.
• it does not exit early, so --allow-error has no effect
• the --strict flag makes any warnings cause an exit with code 1
  (This does not include lexical binding cookie warnings)

Others

• when --json is set, but there are no warnings or errors, output {}
• always print (Checked N files) whether there are errors or not
• ensure (Checked N files) includes files with and without errors

[jcs090218]

I remembered why it was outputted to stderr instead of stdout (sorry, it's been a while 😓) since other CLI tools also use stderr for this purpose; therefore, it was designed this way. Other things to consider, like flycheck-eask wouldn't work after switching to stdout. 🤔

I've temporarily added the function eask-stdout to analyze.el. It should probably go in _prepare.el.

Yes, but I guess we don't need it anymore since it should go to stderr. (above statement) 🤔

How would you suggest to integrate that with the other message commands?

The message commands are a bit out of control. This indeed requires some refactoring due to inconsistencies and a lack of standards. 😅 eask-print, eask-println, eask-write, etc. 😨

• when --json is set, but there are no warnings, output {}
• always print (Checked N files) whether there are errors or not (prints to stderr)
• ensure this counts error files as well

👍 Thanks for taking care of this!

• eask analyze exits with 1 if there are any errors in any of the files checked.
• it does not exit early, so --allow-error has no effect
• currently --strict has no effect either (TODO should it?)

Exiting code with 1 sounds good!

There is a conflict between the options --allow-error and --strict, as the program will be interrupted (reporting an error) while analyzing Eask files. This is a temporary workaround by design for now.

[joshbax189]

Cool! I didn't know about flycheck-eask. If mixed JSON+text output works there, then that's good enough. I'll figure out some way of replicating that in tests.

There is a conflict between the options --allow-error and --strict

I meant that --strict should case it to exit with 1 if there are any warnings. Does that make sense?

[jcs090218]

FYI, there is also flymake-eask too! :D

I meant that --strict should case it to exit with 1 if there are any warnings. Does that make sense?

Oh, yeah! In that case, yes! 👍

[joshbax189]

Ok so I've reverted to use only stderr.

In order to test the JSON output I've made it so all JSON output is on a single line. This matches how flycheck works, and I've tested that flycheck-eask still works in the updated version.

[jcs090218]

The pretty print eask-analyze--pretty-json is probably still better?

[joshbax189]

There needs to be a way to separate JSON from plain text in tests. See the function tryJSON in analyze.test.js. It wont work if the JSON is pretty printed.

I'm assuming that the output from analyze --json is just for flycheck/flymake and so it doesn't need to be pretty printed.

The other alternative would be to write something like this https://stackoverflow.com/a/54608716, which is kind of messy and likely fragile.

[jcs090218]

What if we convert the pretty print result to a single line result in the tryJSON function? 🤔

A printed result is a better UX, since I seldom read the JSON output directly from the terminal. Additionally, it's easier to debug and add new content to the output. :)

[joshbax189]

Ok I made a compromise solution that assumes that all non-JSON lines don't begin with whitespace and there is a single JSON object in the output. See the comments in the code. It allows pretty-printing the output, but the test may fail while flycheck still accepts the output.

[jcs090218]

I wonder why the test may fail. It seems fine and the CI passed! :D

[joshbax189]

I mean, in general, you can change the code in analyze.el so that analyze --json prints output that causes the test to fail while flycheck-eask is still working.

[jcs090218]

Let me know if this is ready to merge! I'll be happy to merge this! :)

[joshbax189]

Yep ready to merge. I've updated the PR description to just show what was actually changed after revisions.

[jcs090218]

Awesome! Merged now! Thank you so much for your hard work! 🚀 🥳