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

Optimized, Beauty, Cleanups, ... #1490

Closed
wants to merge 7 commits into from
Closed

Optimized, Beauty, Cleanups, ... #1490

wants to merge 7 commits into from

Conversation

fishchess
Copy link

@fishchess fishchess commented Dec 18, 2022

before Optimized

 let ret = "";
  for (let i = 0; i < ss.params.length; i++) {
    const p = ss.params;
    ret +=
      p[i].name +
      "," +
      p[i].start +
      "," +
      p[i].min +
      "," +
      p[i].max +
      "," +
      p[i].c.toFixed(2) +
      "," +
      p[i].r.toFixed(5) +
      "\n";
  }
  return ret;

After Optimized

    let ret = "";
    for (let i = 0; i < ss.params.length; i++) {
        const p = ss.params;
        ret += p[i].name + "," + p[i].start + "," + p[i].min + "," + p[i].max + "," + p[i].c.toFixed(2) + "," + p[i].r.toFixed(5) + "\n";
    }
    return ret;

Before Optimized

  if (
    // Safari on iOS doesn't support them
    "Notification" in window &&
    // Chrome and Opera on Android don't support them
    !(
      navigator.userAgent.match(/Android/i) &&
      navigator.userAgent.match(/Chrome/i)
    )
  )
    return true;
  return false;

After Optimized

    if (// Safari on iOS doesn't support them
    "Notification"in window && // Chrome and Opera on Android don't support them
    !(navigator.userAgent.match(/Android/i) && navigator.userAgent.match(/Chrome/i)))
        return true;
    return false;

@ppigazzini
Copy link
Collaborator

Please mind that we use formatters for the all the code base, see #634
For js, html, css:

npx prettier --write "server/fishtest/static/{css/*.css,html/*.html,js/*[!highlight.diff.min].js}"

@fishchess fishchess changed the title Optimized & Beauty JavaScript Code Adaptation to W3C, Optimized, Beauty, Cleanups, ... Dec 18, 2022
@mhouppin
Copy link

Please mind that we use formatters for the all the code base, see #634 For js, html, css:

yes, I see, no formatting changed, no functional changed

Just Optimized & Beauty Code Useful for developers

Formatting Adaptation to W3C

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.
Furthermore, these changes make the code harder to read and maintain.

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.

@fishchess

This comment was marked as abuse.

@mhouppin
Copy link

const padDotVersion = (dn) =>
  dn
    .split(".")
    .map((n) => +n + 1000)
    .join("");

is this formatting is pertinent?

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.

formating2 (formating1 code) ≠ formating2 code

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.

fishtest already uses a tool to format source code

can I see e.g. calc.js source after formatting ?

The file was already formatted by the tool before your PR, so reusing the tool on it will cancel your edits.

Could you please explain the reasoning behind this PR ?

my code formatting is best format, show me a bug in code formatting that not optimized or beauty ?

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.
"Optimizing" a code for LOC is definitely not a good argument here, since it makes the code indigest and complex for no pertinent reason.

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.

@fishchess

This comment was marked as spam.

@mhouppin
Copy link

first format

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);

secound format

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 is quite clear that second format is beauty & optimized, do you really not see?

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".

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.

I completely disagree because if formating2 (formating1 code) = formating2 code then prettier is false formatter

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.

if you right, all 3 values must be same as separated line

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.

@mhouppin
Copy link

not exits a true formatting, one value separate, one value Attached, who do this formatting?!!!!!!

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.

@mhouppin
Copy link

mhouppin commented Dec 18, 2022

@mhouppin there is no formatting do , 3 value are different , you can formatting this code with any tool and you can see All three should be the same

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.

@mhouppin
Copy link

This is wrong, since the auto-formatting tool matches the Fishtest Coding Style Guide, and will keep the initial code block after formatting.

@fishchess
Copy link
Author

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

@MinetaS
Copy link
Contributor

MinetaS commented Dec 18, 2022

There are no absolute must-follow standards about coding styles and conventions.
If you want to propose a new formatting style, please open an issue and provide reasonable grounds.

@mhouppin
Copy link

Ok ,if you right, Fishtest Coding Style has problem because 3 value formats must be as same

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.

@ddobbelaere
Copy link

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.

@fishchess fishchess changed the title Adaptation to W3C, Optimized, Beauty, Cleanups, ... Optimized, Beauty, Cleanups, ... Dec 19, 2022
@peregrineshahin
Copy link
Contributor

Could you stop mentioning me and other people here, please?

@peregrineshahin
Copy link
Contributor

peregrineshahin commented Dec 19, 2022

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
845fa4c
and after the suggestion by vdberg here
#1480 (comment)
"The bad news is that there are quite a few HTML errors at other places. These should be fixed (preferably by the people which are responsible for the current HTML code) but this is outside the scope of this PR."

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 / on void HTML elements because it simply does not have the implementation to check if it's a void element or not.
I pointed to that to the maintainer with

  • Prettier forces the / on void elements.
    in which was his response to:
  • Yes, I read, but we can fix the mako templates that are not formatted with prettier

validator/validator/wiki/Markup-%C2%BB-Void-elements#configuring-tools-to-not-output-trailing-slashes-for-void-elements

moreover, doing the adaptation to that validator we indeed found some html bad practices used.
And as I replaced jQuery to JavaScript in this commit peregrineshahin@911213f
I left out some inconsistencies that are fixed now.

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
Example:

const chart_colors = ["#3366cc", "#dc3912", "#ff9900", "#109618", "#990099", "#0099c6", "#dd4477", "#66aa00", "#b82e2e", "#316395", "#994499", "#22aa99", "#aaaa11", "#6633cc", "#e67300", "#8b0707", "#651067", "#329262", "#5574a6", "#3b3eac", "#b77322", "#16d620", "#b91383", "#f4359e", "#9c5935", "#a9c413", "#2a778d", "#668d1c", "#bea413", "#0c5922", "#743411", "#3366cc", "#dc3912", "#ff9900", "#109618", "#990099", "#0099c6", "#dd4477", "#66aa00", "#b82e2e", "#316395", "#994499", "#22aa99", "#aaaa11", "#6633cc", "#e67300", "#8b0707", "#651067", "#329262", "#5574a6", "#3b3eac", "#b77322", "#16d620", "#b91383", "#f4359e", "#9c5935", "#a9c413", "#2a778d", "#668d1c", "#bea413", "#0c5922", "#743411", ];

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.
sprt.js L57

this.characteristics = function(elo) {
    const score = L(elo);
    const h = (2 * score - (this.score0 + this.score1)) / (this.score1 - this.score0);
    const PT_ = PT(this.LA, this.LB, h);
    return [PT_[0], PT_[1] / this.w2];
}
;

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 assuming you don't like prettier as a formatter there is a better place to discuss it as someone already pointed to you where you can find what we use for formatting.

Now, as I'm pretty sure you are a troll, but sorry I will not further reply to you anywhere anymore.

@fishchess fishchess closed this Dec 19, 2022
@fishchess
Copy link
Author

#1490 (comment) @vdbergh I have completed the supplementary description for this PR, Type A-F, is it possible to be merged?

@ppigazzini
Copy link
Collaborator

Just a note. If prettier had been able to format mako template, I would have definitely preferred automatic formatting to perfect W3C validation.
Before the use of autoformatters the code base was more hard to be read.

@fishchess
Copy link
Author

Before the use of autoformatters the code base was more hard to be read.

prettier is good formatter but my formatter is much better than prettier

You can also pass my codes through prettier filter, there will be no changes in the codes

You can see the huge difference in the codes (A-F types) , prettier is loser vs my formatter

@fishchess fishchess reopened this Dec 19, 2022
@vdbergh
Copy link
Contributor

vdbergh commented Dec 19, 2022

Stephane always points to this https://tirania.org/blog/archive/2010/Dec-31.html .

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.

8 participants