-
Notifications
You must be signed in to change notification settings - Fork 132
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
Optimized, Beauty, Cleanups, ... #1490
Conversation
Please mind that we use formatters for the all the code base, see #634
|
All your changes are basically formatting changes. As explained by @ppigazzini , fishtest already uses a tool to format source code, which makes your edits unuseful since they will be all reverted by the next PR. Could you please explain the reasoning behind this PR ? There's little to no chance this will get merged, unless you provide clear insights as to why this reformatting is pertinent. |
This comment was marked as abuse.
This comment was marked as abuse.
Yes. It's a very common thing in functional languages to place chained calls like this on a separated line, as it makes it much easier to see how a variable is manipulated to go from one state to another. This is for example the standard with Rust default formatter when these constructs take too much place to fit on one line.
In the case of your PR, this is wrong. You didn't change anything besides code indentation and spacing, so the next PR using the formatting tool will revert all the changes.
The file was already formatted by the tool before your PR, so reusing the tool on it will cancel your edits.
There's no such thing as "best format" or "beauty", and coming on a repository to argue about the formatting on your first PR is not how you should contribute to an open-source project. I'll let maintainers take the final stance, but I think this PR can be closed, as it doesn't bring value to the project and doesn't match the Fishtest Coding Style Guide. |
This comment was marked as spam.
This comment was marked as spam.
Nope. Making convoluted code doesn't make it more "beautiful" or "optimized", it's literally the same thing, but less readable. Once again, open-source projects care about maintainability, not "beauty".
Disagreeing with what I said doesn't make my argument wrong unfortunately. Changing where whitespaces are placed in the code will mean nothing once the formatter processes the JS file again, reverting everything back to its state, so your edits are essentially a no-op.
Your sentence doesn't make any sense. Once again, this PR doesn't match the Fishtest Coding Style Guide, and once again, arguing about formatting is not how you should contribute to an open-source project. |
Literally every open-source Rust project will have this formatting, and I would expect most major JavaScript projects to have a similar (albeit maybe slightly different) code format for the sake of readability. |
const getCellValue = (tr, idx) =>
tr.children[idx].dataset.diff ||
tr.children[idx].innerText ||
tr.children[idx].textContent;
const padDotVersion = (dn) =>
dn
.split(".")
.map((n) => +n + 1000)
.join("");
const padDotVersionStr = (dn) => dn.replace(/\d+/g, (n) => +n + 1000); This code block is already formatted, and the three variables follow the same rules for their formatting. You still haven't acknowledged that a tool for auto-formatting was already on the repo (which is the exact reason these three variable assignations are formatted as-is), and that Fishtest already had a Coding Style Guide. In its state, this PR will not get merged. |
This is wrong, since the auto-formatting tool matches the Fishtest Coding Style Guide, and will keep the initial code block after formatting. |
Ok ,if you right, Fishtest Coding Style has problem because 3 value formats must be as same |
There are no absolute must-follow standards about coding styles and conventions. |
You might consider it as an issue, but this isn't the correct place to argue about it. I would again suggest reading this article that explains well why formatting changes are very often not well received by project maintainers, before you decide to create an issue about it. |
Don't feed the troll, see https://en.m.wiktionary.org/wiki/don%27t_feed_the_troll He's given ample proof of himself earning the troll status, not only in this joke PR (downvoted by himself btw), but also in other numerous irrelevant "issues", PR discussions, etc. What a complete waste of time and energy. Get a life, man. Seriously. |
Could you stop mentioning me and other people here, please? |
I'm not sure I should feed the troll-ness of yours after the maintainer himself told you that for JavaScript we use Prettier which did the formatting for us, the reason why I did the formatting and a cleanup PR is the maintainer himself made the decision to adapt to the W3C validator in these two commits eb9de7c Now the exception made here to not use prettier is because prettier first can't format the mako templates that have JavaScript, CSS, and HTML, but also another exception is that Prettier formatter forces a
moreover, doing the adaptation to that validator we indeed found some html bad practices used. Now looking at your PR it has bad formatting indeed. since all it does is instead of splitting lines for readability as suggested by our tool used Prettier, with yours it all dumbing code in a one-line based on the naïve understanding that this optimizes the code for whatever reason
your code has this one goes of the screen for whatever reason, do you really want me to scroll horizontally so see the end of the line in hopes that this optimizes some magical performance, no thanks also, I'm not sure I understand the technical reason why the formatting has a semi-colon floating alone on a new line.
Now, assuming you are not a troll but a really naïve beginner in software engineering and open-source community here is a good article on how to not contribute to the open-source community tirania.org/blog/archive/2010/Dec-31.html Now, as I'm pretty sure you are a troll, but sorry I will not further reply to you anywhere anymore. |
#1490 (comment) @vdbergh I have completed the supplementary description for this PR, Type A-F, is it possible to be merged? |
Just a note. If |
You can also pass my codes through You can see the huge difference in the codes (A-F types) , |
Stephane always points to this https://tirania.org/blog/archive/2010/Dec-31.html . |
before Optimized
After Optimized
Before Optimized
After Optimized