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

Prettier 3.0 #8030

Merged
merged 5 commits into from
Jul 10, 2023
Merged

Prettier 3.0 #8030

merged 5 commits into from
Jul 10, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Jul 10, 2023

Description

https://prettier.io/blog/2023/07/05/3.0.0.html

Also has a 'yarn rebuild' of cosmic-proto that was affected by #7822

uses alpha version of eslint-plugin-prettier due to prettier/eslint-plugin-prettier#562

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

CI

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about the generated declare things, but I suppose the tools will catch them next time too.

Comment on lines +158 to +162
console.warn('running with options', opts);
const { agoricNames, lookupPsmInstance } = await rpcTools();
const instance = await lookupPsmInstance(opts.pair);
const offer = Offers.psm.swap(agoricNames, instance, opts);
outputExecuteOfferAction(offer);
Copy link
Member

Choose a reason for hiding this comment

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

sigh... indenting the whole body of the function is mildly annoying. If I saw this while writing code, I might shift things around.

declare type Builtin =
type Builtin =
Copy link
Member

Choose a reason for hiding this comment

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

ugh. this code is generated by some protobuf tool. next time we run that tool (yarn generate) it's going to say declare again, IIUC. I wonder if this is a known issue upstream in the protobuf tool.

cc @kriskowal @mhofman

Copy link
Member Author

Choose a reason for hiding this comment

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

this was generated by yarn rebuild which runs yarn generate. I put this into a separate commit, before changing Prettier: 87a2df6

? object.messages.map((e) => String(e))
? object.messages.map(e => String(e))
Copy link
Member

Choose a reason for hiding this comment

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

again, this is generated code. Any clues on why our lint tools didn't complain about it when it was originally merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is the generated code after being prettiered (yarn rebuild runs prettier). The change in b46c46 is to use the same Prettier config for this package as for agoric-sdk, i.e. "arrowParens": "avoid"

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, are you saying the codegen steps already re-format generated code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

"rebuild": "rm -rf gen && mkdir -p gen && yarn generate && tsc --build && yarn prettier -w dist/agoric && rm -rf gen",

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Linting tools should not touch generated code

@mhofman mhofman dismissed their stale review July 10, 2023 18:33

Not a new behavior apparently

@turadg turadg enabled auto-merge July 10, 2023 18:36
@turadg turadg added this pull request to the merge queue Jul 10, 2023
Merged via the queue into master with commit 3c22e86 Jul 10, 2023
@turadg turadg deleted the prettier-3.0 branch July 10, 2023 19:04
mhofman pushed a commit that referenced this pull request Aug 7, 2023
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.

3 participants