rename prettier command to format/reenable prettier in CI by scott-silver · Pull Request #132 · compound-finance/comet

scott-silver · 2022-01-25T22:31:28Z

The script alias that we have for prettier shadows the name of the library itself:

# package.json
  "scripts": {
    // ...
    "prettier": "prettier --ignore-unknown --write",

So if you wanted to do a check for any files that do not pass prettier, and you ran yarn run prettier --check ., the actual command that would run would be prettier --ignore-unknown --write --check .

That's what's happening with the Github Action command in run-prettier.yaml. Adding the --write flag means that this command doesn't exit with status 1 when there are un-prettified files, so the check always passes.

Basically, we haven't been enforcing that prettier is run, so we have a bunch of pending prettier changes.

If we rename this script to anything other than prettier (I've used format, but I'm open to suggestions), then the Github Action will actually start enforcing that we're using prettier.

Alternatively, we could just delete prettier.

My general perception is that it has been more of a nuisance than a help. This change is going to make it a much bigger nuisance, since we'll actually start enforcing that it's been used before merging.

jflatow · 2022-01-25T23:12:57Z

.github/workflows/scenario-report.yaml

+          artifact: scenario-results # Artifact name
+          name: Scenario Tests # Name of the check run which will be created
+          path: 'scenario-results.json' # Path to test results (inside artifact .zip)
+          reporter: mocha-json # Format of test results


This one looks a little opposite of prettier

jflatow · 2022-01-25T23:14:38Z

docs/README.md

 ```js
 const comet = new web3.eth.Contract(abiJson, contractAddress);
-await comet.methods.absorb("0xUnderwaterAddress...").send();
+await comet.methods.absorb('0xUnderwaterAddress...').send();


I like some of these (like this one), but others are kind of meh. Maybe the answer is to dial down the rules a bit?

yeah, we can definitely disable some of the rules.

Prettier deliberately limits how much you can configure it (https://prettier.io/docs/en/option-philosophy.html), so there will be a limit to how much we can dial it down.

The other option would be to start using something like ESLint instead. It might give us better control over defining the style that we want our files to conform to, and a little more flexibility in terms of where we add line breaks, put comments, etc.

Yeah I suppose we should add it back, maybe after we merge the current round of other outstanding PRs though?

sure, no hurry

jflatow · 2022-01-25T23:15:01Z

package.json

    "lint-contracts": "solhint 'contracts/**/*.sol'",
    "lint-contracts:fix": "solhint --fix 'contracts/**/*.sol'",
-    "prettier": "prettier --ignore-unknown --write",
+    "format": "prettier --ignore-unknown --write",


Good catch 🎣

jflatow · 2022-01-25T23:15:47Z

plugins/deployment_manager/Utils.ts

 async function asAddresses(contract: Contract, fnName: string): Promise<Address[]> {
-  if (fnName.startsWith('%')) { // Read from slot
+  if (fnName.startsWith('%')) {
+    // Read from slot


This one's a little obnoxious

jflatow · 2022-01-25T23:15:53Z

plugins/deployment_manager/Utils.ts

    throw new Error(`Cannot find contract function ${contract.name}.${fnName}()`);
  }
-  let val = (await fn());
+  let val = await fn();


jflatow · 2022-01-25T23:17:00Z

plugins/deployment_manager/Utils.ts

+      `Could not find contract ${targetContract} in buildFile with contracts: ${JSON.stringify(
+        Object.keys(contracts)
+      )}`
+    );


Longer lines would get rid of this one?

Also, I might die trying to handle this merge, since this file has been totally rewritten.

jflatow · 2022-01-25T23:18:13Z

plugins/scenario/Runner.ts

 function bindFunctions(obj: any) {
  for (let property of Object.getOwnPropertyNames(Object.getPrototypeOf(obj))) {
-    if (typeof(obj[property]) === 'function') {
+    if (typeof obj[property] === 'function') {


Controversial maybe

jflatow · 2022-01-25T23:18:25Z

plugins/scenario/Stack.ts

+  function?: string;
+  file?: string;
+  line?: number;
+  char?: number;


jflatow · 2022-01-25T23:18:31Z

plugins/scenario/Stack.ts

  let index = 0;

-  while (null != (next=regex.exec(stack))) {
+  while (null != (next = regex.exec(stack))) {


hayesgm · 2022-01-31T19:36:00Z

Out of curiousity, why not just run npx prettier ... which wouldn't run the script command, right?

scott-silver · 2022-02-01T04:11:35Z

@hayesgm yep, that would work, too. we could also reference the npm module directly:

./node_modules/prettier/bin-prettier.js --check .

I opted to change the command because the error would anywhere someone ran yarn prettier; seemed like surprising behavior.

The more I've thought about it, the more I think ESLint might be a better fit for us.

Prettier seems like it's well-suited to large teams with developers of varying skill levels. We have a small team of senior people. I think the customizability of ESLint might be more appropriate.

hayesgm

Looks great; biggest concern is can we wait for a few larger PRs to drop before we merge this? Just going to be a big conflict with some of the files that are coming in.

scott-silver · 2022-03-31T22:13:33Z

closing for now

jflatow · 2022-03-31T23:40:40Z

we don't still want to do this eventually?

rename prettier command to format

6fae579

scott-silver requested review from hayesgm, jflatow and kevincheng96 January 25, 2022 22:31

scott-silver mentioned this pull request Jan 25, 2022

fix or skip failing scenarios #131

Closed

run format

1fceb1f

jflatow reviewed Jan 25, 2022

View reviewed changes

plugins/scenario/Stack.ts

function?: string;

file?: string;

line?: number;

char?: number;

Copy link
Copy Markdown

Contributor

jflatow Jan 25, 2022

Choose a reason for hiding this comment

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

Nice

jflatow reviewed Jan 25, 2022

View reviewed changes

scott-silver changed the title ~~rename prettier command to format~~ rename prettier command to format/reenable prettier in CI Jan 25, 2022

hayesgm requested changes Feb 3, 2022

View reviewed changes

scott-silver closed this Mar 31, 2022

scott-silver mentioned this pull request Apr 21, 2022

replace Prettier with ESLint #321

Merged

jflatow deleted the silver/fix-prettier-command branch May 26, 2022 05:15

Conversation

scott-silver commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hayesgm commented Jan 31, 2022

Uh oh!

scott-silver commented Feb 1, 2022

Uh oh!

hayesgm left a comment

Choose a reason for hiding this comment

Uh oh!

scott-silver commented Mar 31, 2022

Uh oh!

jflatow commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

scott-silver commented Jan 25, 2022 •

edited

Loading