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

🛠️ Repo: Pointless function in nyancat reporter implementation #2916

Closed
ScottFreeCode opened this issue Jul 8, 2017 · 3 comments · Fixed by #5056
Closed

🛠️ Repo: Pointless function in nyancat reporter implementation #2916

ScottFreeCode opened this issue Jul 8, 2017 · 3 comments · Fixed by #5056
Assignees
Labels
area: reporters involving a specific reporter status: accepting prs Mocha can use your help with this one! type: chore generally involving deps, tooling, configuration, etc.

Comments

@ScottFreeCode
Copy link
Contributor

ScottFreeCode commented Jul 8, 2017

Some of our reporters use console.log, others use process.stdout.write. Nyancat uses write. What is write?

function write (string) {
  process.stdout.write(string);
}

This isn't being used to pass process.stdout.write into anything else without having to bind it to process.stdout, it's just being called directly, so all it does is omit the text process.stdout.. Furthermore, I reviewed the file's commit history and wasn't able to find any evidence that it was ever used differently than it is now, nor was it at any point implemented differently (the only change in these three lines was to add a space after the function name), and it goes back to the reporter's original commit.

(In terms of metrics, this function plus its JSDoc adds nine lines to the file, but it's only about 150 bytes while it would be about 350 bytes to add process.stdout. to every write(...) call, thus it costs 9 lines and saves about 200 bytes. Just in case anyone cares. I don't think it matters to anything but coverage line and function counts.)

If this were something any of the other reporters were also doing, I'd chalk it up to being a preferred style; if the write function were in its own module and being used by other reporters, I'd say cool, we can make any updates to the implementation in just one place... As is, it just looks like a needless function that needlessly makes this reporter's implementation look different from the others when it really isn't. (Then again, maybe if it were var write = process.stdout.write; up front, a la x = require("whatever").x, I'd have a different impression despite that being technically nearly identical assuming it works at all [no binding needed].)

Anyone feel strongly for or against removing it? Part of me doesn't want to clutter the file's blame history with a change from write to process.stdout.write all over. But it seems like an obvious cleanup.

@ORESoftware
Copy link

ORESoftware commented Jul 9, 2017

Not sure if my opinion counts, but I definitely think this process.stdout.write helper function is totally fine in pretty much any .js file you like. But if it's causing problems for JSDoc, I'd have to see how much of a headache it is there.

@boneskull
Copy link
Contributor

related? #2943

@boneskull boneskull added the type: chore generally involving deps, tooling, configuration, etc. label Oct 6, 2017
@ScottFreeCode
Copy link
Contributor Author

Arguably so.

@Bamieh Bamieh added the area: reporters involving a specific reporter label Nov 18, 2018
@JoshuaKGoldberg JoshuaKGoldberg changed the title pointless function in nyancat reporter implementation 🛠️ Repo: Pointless function in nyancat reporter implementation Dec 27, 2023
@JoshuaKGoldberg JoshuaKGoldberg self-assigned this Dec 27, 2023
@JoshuaKGoldberg JoshuaKGoldberg added the status: accepting prs Mocha can use your help with this one! label Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: reporters involving a specific reporter status: accepting prs Mocha can use your help with this one! type: chore generally involving deps, tooling, configuration, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants