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

Update files with prettier formatting #668

Merged

Conversation

HaudinFlorence
Copy link
Contributor

Update the .ts, .js, .json, .css and .md files with prettier formatting.

.prettierrc Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@vidartf
Copy link
Collaborator

vidartf commented May 6, 2023

Thanks! I left some comments on the config. Additionally it would probably make sense to ignore the package-lock.json file, as it is auto-generated.

@HaudinFlorence
Copy link
Contributor Author

@vidartf Sorry for the multiple commit reverts. It is to keep on having all the formatting in a single commit and I forgot to include your suggestion about ignoring the package-lock.json.

Copy link
Collaborator

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

This looks a lot better now! The TS files particularly are ok, but I wonder if the new rules align with what is encoded in the tslint config files? Otherwise some small comments and questions.

packages/nbdime/tslint.json Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
.nbdime-root.jp-mod-hideUnchanged .jp-Diff-addremchunk[data-nbdime-NCellsHiddenBefore]::before {
content: attr(data-nbdime-NCellsHiddenBefore) " unchanged cell(s) hidden";
.nbdime-root.jp-mod-hideUnchanged
.jp-Cell-diff[data-nbdime-NCellsHiddenBefore]::before,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer it if single CSS rules weren't split across lines (if such a rule config is easily available.

Copy link
Contributor Author

@HaudinFlorence HaudinFlorence May 11, 2023

Choose a reason for hiding this comment

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

I have tried to add --print-width=120 in prettier script options in the package.json file but it doesn't seem to produce effect on the width of the lines.

@vidartf
Copy link
Collaborator

vidartf commented May 10, 2023

Note on git techniques: When we agree on the final config, we can use git rebase -i master. This should allow for combining the different config commits + formatting commits into one commit each. Then it can be force-pushed with git push -f.

@@ -0,0 +1,5 @@
{

Choose a reason for hiding this comment

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

Some thoughts:

  • this (but not the .prettierignore grumble grumble) could live in package.json#/prettier, avoiding Yet Another File in the root of the repo
  • if also using black, setting columnWidth: 88 is nice, and can often make a difference for long ts signatures
  • since this will already change a lot of files, adding proseWrap: always (only applies to markdown) can further tighten up diffs

import type {
JupyterFrontEndPlugin, JupyterFrontEnd
JupyterFrontEndPlugin,

Choose a reason for hiding this comment

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

adding https://github.com/trivago/prettier-plugin-sort-imports , with a bit of config, can help substantially for big import lists e.g. "first unknown-third-party, then lumino, then lab, then local"

@HaudinFlorence HaudinFlorence force-pushed the update_files_with_prettier_formatting branch from cd7994f to e21a723 Compare May 11, 2023 22:29
@vidartf vidartf marked this pull request as ready for review July 7, 2023 19:12
@fcollonval
Copy link
Collaborator

Thanks @vidartf for pushing on this PR. Could we do anything more to help you merging this one?

@vidartf
Copy link
Collaborator

vidartf commented Jul 10, 2023

No, I think this PR is ready to go, it is just a question of checking the timing vs other PRs in order to minimize conflict resolution work.

@vidartf vidartf merged commit 43168fc into jupyter:master Sep 8, 2023
@krassowski
Copy link
Member

@vidartf I wonder if that would have been easier the other way (first merging the lab 4.0 PR and then applying formatting)?

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.

5 participants