tools: add message on auto-fixing js lint issues in gh workflow

[dario-piotrowicz]

For cpp formatting the github workflow prints a helpful message suggesting how to fix the issue:

I figured it could be nice to have the same for the js linting.

Before

After

[nodejs-github-bot]

Review requested:

• @nodejs/actions

[aduh95]

wdyt about running it in the CI and show the diff like the C++ linter does?

[dario-piotrowicz]

Sorry I don't think I am familiar with C++ one, should you share a link with a run? 🙏

[aduh95]

You're the one who shared a screen shot in the OP 😅

[dario-piotrowicz]

Ah sorry! that's what you meant! 🤦

Sorry I didn't follow that 🤦 (when you said CI I thought you were referring to some Jenkins runs 🙈)

Mh... I'm not sure, if you think it'd be a nice addition I could look into that, but eslint already gives such good logs for the errors that adding a diff there, in my opinion, could add extra (unnecessary) noise and make the eslint errors less visible 🤔

Moreover (please tell me if I'm wrong) I think that format-cpp fixes all cpp formatting issues right? while eslint --fix is not guarantee to fix all the issues, so there's this difference here (so a diff here could be only partially correct)

[aduh95]

while eslint --fix is not guarantee to fix all the issues

Sure but it's easy to check whether it could: just check the exit code – IMO that would be a useful info to tell the user whether running that command will solve all the issues

Suggested change

	echo ' Please fix the lint errors.'
	echo ' Run:'
	echo ' make lint-js-fix'
	echo ' to fix the auto-fixable lint issues.'
	echo ' Please fix the lint errors.'
	echo ' Run:'
	echo ' make lint-js-fix'
	if make lint-js-fix; then
	echo ' to fix the lint issues.'
	git diff
	elif git diff --quiet --exit-code; then
	echo ' to fix none of the lint issues, manual fixes are required.'
	else
	echo ' to fix some of the lint issues, some manual fixes are also required.'
	fi

[dario-piotrowicz]

I totally like the idea of the different messages! 🫶

Regarding showing the diff I am still unsure about that to be honest... let me have a quick play with that, as I mentioned I do personally feel like that would likely add more noise than help

In an case, Just so that I understand your point of view, how do you expect users to benefit/use the diff? would it be for informative purposes? (wouldn't they just see the same after running make lint-js-fix anyways?) or do you think that they could just copy-paste the diff into their local files? (but why not just running make lint-js-fix at that point?), or something else?
I am really not trying to criticize the idea, I'm just trying to understand the purpose/use case of the diff 🙂

[aduh95]

For the C++ linter, I've found the diff useful so I can make my changes using the web UI (e.g.PR suggestion), so I don't have to check out the branch locally and run the linter myself – I can even do that from mobile. I admit that in most cases, the ESLint error message is enough for me to know what diff I should apply, but also I have a few years of experience with the codebase and ESLint.
I don't feel strongly about it, feel free to ignore my suggestion, IMO this PR is already an improvement with or without the diff.

[dario-piotrowicz]

@aduh95 so sorry for the late reply 🙇

I understand your use case, I think it's not something for me but it's definitely valid and interesting 🙂

I've applied the changes you suggested alongside some minor fixes/improvements, please have another look at at PR when you've got time 🙏

For your convenience here's the output in the various cases:

• there are no js lint issues
• all the js lint issues are auto-fixable
• some issues are auto fixable and some are not
• all issues are not auto-fixable

[dario-piotrowicz]

PS: the text messages and formatting could be cleaned up a bit, I can look into that tomorrow, but as for the general direction, is this the sort of thing you had in mind? 🙂