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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: format all possible files with prettier #2172

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Apr 26, 2022

As I was implementing #2171, I noticed that a few of the files I was touching were expecting to be formatted differently due to the presence of .prettierrc.cjs. This is a setting I choose to have in my editor, and it was easy enough to turn it off for this project (which is what I did).

But, I figured the presence of .prettierrc.cjs signaled intent to have prettier format at least some of the files in this repo. This PR takes that to the extreme and implements formatting for all file types prettier handles, and ignores only built files.

I know this is quite a bit of churn so I'm happy to either:

  1. close this 馃槃
  2. fixup with more entries in .prettierignore.cjs
  3. fixup with some file types excluded

@bcoe
Copy link
Member

bcoe commented May 14, 2022

This PR takes that to the extreme and implements formatting for all file types prettier handles, and ignores only built files.

I see no reason not to do this, let's see what happens.

@bcoe
Copy link
Member

bcoe commented May 16, 2022

@lukekarrys I'd love to drive down the other open PRs, before we land this change (as it's likely to cause conflicts).

@lukekarrys
Copy link
Contributor Author

I'd love to drive down the other open PRs, before we land this change (as it's likely to cause conflicts).

@bcoe Sounds like a good plan to me. I'd be happy to fix any conflicts and squash when it's time.

@shadowspawn
Copy link
Member

  1. I have a minor reservation about the changes to code blocks in markdown files. Some of the changes are blowing out reasonable compact layout for examples, or breaking the chaining pattern. However, may still be a win with consistency.

e.g. notice .argv moving, and functions expanding:

// before
 var argv = require('yargs/yargs')(process.argv.slice(2))
     .count('verbose')
     .alias('v', 'verbose')
     .argv;

 VERBOSE_LEVEL = argv.verbose;

 function WARN()  { VERBOSE_LEVEL >= 0 && console.log.apply(console, arguments); }
 function INFO()  { VERBOSE_LEVEL >= 1 && console.log.apply(console, arguments); }
 function DEBUG() { VERBOSE_LEVEL >= 2 && console.log.apply(console, arguments); }
 // after
var argv = require('yargs/yargs')(process.argv.slice(2))
   .count('verbose')
   .alias('v', 'verbose').argv;

 VERBOSE_LEVEL = argv.verbose;

 function WARN() {
   VERBOSE_LEVEL >= 0 && console.log.apply(console, arguments);
 }
 function INFO() {
   VERBOSE_LEVEL >= 1 && console.log.apply(console, arguments);
 }
 function DEBUG() {
   VERBOSE_LEVEL >= 2 && console.log.apply(console, arguments);
 }

(Hmm, I wonder if replacing .argv with .parse() would avoid that particular reformat!)

  1. The current lint setup does not check any of the "other" files. That may be out of scope for gts given its name. 馃槃

@bcoe
Copy link
Member

bcoe commented Apr 28, 2023

@lukekarrys is this cleanup still something you're interested in doing, sorry this dropped on the floor.

Any thoughts regarding @shadowspawn's feedback?

@lukekarrys
Copy link
Contributor Author

lukekarrys commented May 16, 2023

@bcoe yeah, i'd love to get this pushed across the finish line.

@shadowspawn it would be easy to ignore markdown files altogether, but i agree that consistency here is valuable. i've also found value in the past in formatting basic markdown examples in order to catch typos that would result in syntax errors.

i did paste that particular example into the prettier repl and you are right that using .parse() makes that particular case format as desired. i find the rest of the changes to be beneficial in that case. i'll also note that it is possible to increase the line length of the formatter, but all that does in this case is make it possible for the first line to work with no line breaks, which i dont think is ideal.

linting markdown examples, goes to your second point about gts linting other files. i think that would be valuable too, but i think it should be outside the scope of this PR in order to get this very diff heavy PR hopefully merged.

@lukekarrys
Copy link
Contributor Author

i am going to continue this PR by resolving conflicts and rebasing against the latest main

@lukekarrys
Copy link
Contributor Author

lukekarrys commented May 16, 2023

i force pushed to my branch and split this up into three commits:

  1. 6202031: this creates a .prettierignore file and creates/updates the necessary npm run scripts
  2. df03ab4: this runs npm run fix:format and commits the result. the initial run of this command was able to format everything except example/yargs.html which had a syntax error (missing <html> tag)
  3. 2c456f3: npm run fix was fixing a test case, resulting in that test failing. this commit fixes that.

Copy link
Member

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me

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.

None yet

3 participants