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

Run Prettier on All Files #960

Merged
merged 10 commits into from
Sep 19, 2018
Merged

Run Prettier on All Files #960

merged 10 commits into from
Sep 19, 2018

Conversation

benshope
Copy link
Contributor

This runs the prettier on all the files.

Some background, there were some React 16 console errors related to attribute names that were bugging me https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html. I decided to see if there was a codemod from https://github.com/reactjs/react-codemod that would fix all console errors at once. It looks like the codemods run prettier - so it is necessary to get consistency with prettier before trying the codemods.

@mcnuttandrew let me know if I got the prettier settings wrong or something because this seems like a lot of modified files

@mcnuttandrew
Copy link
Contributor

I gave a cursory scroll through all the prettier changes and they all appear to be in order. I don't really know much about using prettier, but it appears that the config you have now is maybe in disagreement with the react-vis lint configuration? It looks like almost all of the lint failures are about indentation stuff of lineoperator-linebreak.

I zero percent care about the style of those elements, so you should feel empowered either to a) adjust the prettier config accordingly b) change the lint config. The one exception is to this is in the history showcase example, which seems to have a regex error of some sort?

Aside: Do lint and prettier play nice together? If prettier is installed do we still use lint for static checking? Or does prettier do everything?

@benshope
Copy link
Contributor Author

Yep, prettier and lint were fighting each other. Just fixed that issue - sorry for diffing a bit prematurely. And prettier is amazing if you are inclined to try it out! It'll change your life.

Prettier and lint play very nicely together. Prettier auto-fixes a large subset of the linter's rules so you run into way fewer lint issues. Their rules do need to not conflict with each other though.

Copy link
Contributor

@mcnuttandrew mcnuttandrew left a comment

Choose a reason for hiding this comment

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

I tried to look through a bunch of it, and it all appears to be in working order. It's a little bit much to view granularly, so I guess thank god for tests.

It's such a pretty code base now! Or at least very consistant. Thanks for doing this


const lineData = [...Array(3).keys()].map(() =>
[...Array(10).keys()].map((x) => ({x, y: Math.random() * 10})));
[...Array(10).keys()].map(x => ({x, y: Math.random() * 10}))
Copy link
Contributor

Choose a reason for hiding this comment

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

okay i wasn't sold about prettier but now i am

@@ -51,7 +43,7 @@ const labelData = greenData.map((d, idx) => ({
export default class Example extends React.Component {
state = {
useCanvas: false
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

i did not know that was legal syntax

@benshope
Copy link
Contributor Author

@mcnuttandrew thanks for stamping! I'll do some manual testing before merging. Always good to do some sanity checking for something like this

@mcnuttandrew
Copy link
Contributor

+10000000000 for sanity checking

@benshope
Copy link
Contributor Author

Checked it out this morning. Should be good. Merging 🤞

@benshope benshope merged commit 35c8950 into master Sep 19, 2018
@benshope
Copy link
Contributor Author

Heads up that this made a small issue for me while link-ed. I pushed a revert of the problematic bit

@benshope benshope deleted the run_prettier branch September 19, 2018 19:55
ayarcohaila pushed a commit to ayarcohaila/react-vis that referenced this pull request Jun 30, 2021
ayarcohaila added a commit to ayarcohaila/react-vis that referenced this pull request May 30, 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.

2 participants