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

Add (inactive) support for auto-formatting on commit #1501

Merged
merged 6 commits into from
Oct 24, 2017

Conversation

sophistifunk
Copy link
Collaborator

Description

Add some scripting that when activated will automatically format staged JS files on commit.

This should have very little effect on anybody unless you specifically symlink the /bin/pre-commit.js file from .git/hooks/pre-commit -- and once this goes into master, I'll be looking for one or two suckers volunteers to do so, to help me stress-test it for a week before we recommend it to everybody.

I have also removed the submodule-specific .eslintrc files, as they only confuse things.

Current plan:

  1. Get this looked at by you mob :)
  2. Merge this to master, and I (and hopefully a volunteer) will do some unrelated development during the next week or so to give it a shake-down, and make sure there's no unintended effects.
  3. Get the rest of the team to symlink the hook in after the next time they pull / rebase from master.
  4. From this point on, files we're editing will get consistently formatted.
  5. After a few weeks, which will allow everybody's changes to go through the formatting to cut down on merge conflicts, I'll reformat the entire source with a batch-change pull-request

I know that most of us are unhappy about the way prettier currently formats object-destructure assignments when they're not quite wide enough to exceed the line limit. We can address this through the prettier.js dev team (and I'll look into how hard it is to cook up a PR for that functionality). Meanwhile in particularly egregious instances, you can insert a //prettier-ignore comment above the statement and that AST node should remain unmolested.

commit 22ccf0537fea65db7068561a360ed7abd6bdf8c2
Author: Josh McDonald <[email protected]>
Date:   Fri Oct 20 16:24:33 2017 +1000

    task/prettify * Revert the module package.jsons

commit 4fc4ad3713b6816c9c56b2d413b43ab22e138522
Author: Josh McDonald <[email protected]>
Date:   Fri Oct 20 16:15:59 2017 +1000

    Revert a source file I accidentally committed earlier

commit 292cb204c62370ddf15a2a46263960f12d3aa76b
Author: Josh McDonald <[email protected]>
Date:   Fri Oct 20 16:11:36 2017 +1000

    task/prettify * Should be working now, as well as reformatting itself as part of this commit :D

commit 3ebad59d6a6b777f9c628365a3890c74e9cb1d9a
Author: Josh McDonald <[email protected]>
Date:   Thu Oct 19 13:59:15 2017 +1000

    should not commit

commit 5c925e3f448e47b7dc02cefd4dc110866698b8c9
Author: Josh McDonald <[email protected]>
Date:   Thu Oct 19 13:58:03 2017 +1000

    should not commit

commit f7f7fa3fdad419e31bf14051ad762026b6bb7eb4
Author: Josh McDonald <[email protected]>
Date:   Thu Oct 19 13:29:02 2017 +1000

    Add hashbang

commit a67b1b041dc26214acfb19e2dbd593fdbf42f7a5
Author: Josh McDonald <[email protected]>
Date:   Thu Oct 19 12:13:08 2017 +1000

    more noise

commit 2d5a0d0950137c906387065c40d77975e49b0877
Author: Josh McDonald <[email protected]>
Date:   Thu Oct 19 12:09:49 2017 +1000

    should not work

commit 59dd8c79da65372f02318816ec39e3b99d82d777
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 18 18:30:12 2017 +1000

    task/prettify * run on self

commit 8e713a9827dc41f2a66c022d74f30baa882d5a60
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 18 18:25:38 2017 +1000

    task/prettify * Fix an issue where usage wouldn't always show

commit 10e1253d4640da6ff343760c6061c58065debd0a
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 18 18:02:26 2017 +1000

    task/prettify * fix some more issues

commit a6d721d809d110a7f2dc1204610bdea2da75a94f
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 18 17:48:01 2017 +1000

    task/prettify * Fix some issues, clean up a bit

commit 19e93ce23e90e38b4947d00a3eb23db7cb242e4f
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 18 17:41:00 2017 +1000

    task/prettify * Add the ability to handle multiple file globa

commit c88673731a5f4cb3850165b448061018e2834cb3
Author: Josh McDonald <[email protected]>
Date:   Fri Oct 13 10:34:20 2017 +1000

    task/prettify * Changes to configuration to add extension type (not used yet)

commit 486382b8314cdd9ec2624d977793c432e65d0f1e
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 11 13:26:38 2017 +1000

    task/prettify * Change so errors don't stop the reporting on the rest of the process

commit 53b8aa1cc33818210c0d195854aebd0e96079c42
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 11 13:08:02 2017 +1000

    task/prettify * Remove eslintrc from core-js and dashboard

commit f6f1f5c59981d81dbfb00da103e024510d824ca6
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 11 12:57:32 2017 +1000

    task/prettify * More slight improvements to script, remove jdl eslintrc

commit 9d872bceb16fda4548fe1054667717535f444119
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 11 11:15:14 2017 +1000

    task/prettify * fix src path in dashboard

commit fffa58e928901627be7975334e6a31bcb15df425
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 11 11:08:21 2017 +1000

    task/linkify * Improve output slightly

commit af8ce728c54f69d7025a7b4f4e5b85fffcde35a4
Author: Josh McDonald <[email protected]>
Date:   Wed Oct 11 11:04:32 2017 +1000

    Add pretty to core-js and dashboard package.jsons

commit 6bb373a6994a400e360c2520756a9b8d566f348e
Author: Josh McDonald <[email protected]>
Date:   Tue Oct 10 19:07:46 2017 +1000

    task/linkify * Switch to babylon parser, clean up tool slightly

commit 7d1fa4f67dfcb0c3885400260ff1dde23363f8b1
Author: Josh McDonald <[email protected]>
Date:   Tue Oct 10 17:48:36 2017 +1000

    task/linkify * Update package to run fix, as well as config

commit 2bd955019efda9dc5a308082b72e59cb9a6cb885
Author: Josh McDonald <[email protected]>
Date:   Tue Oct 10 17:40:31 2017 +1000

    task/linkify * Update on saving

commit f8372d0c92fc53c01964b58797762d09775d5158
Author: Josh McDonald <[email protected]>
Date:   Tue Oct 10 15:48:32 2017 +1000

    task/linkify * Good progress with linkify, time to start writing out some files
@@ -0,0 +1,13 @@
# .prettierrc
printWidth: 160
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still opposed to this. Wider than github, makes reviewing changes in PRs more irritating.

Copy link
Member

Choose a reason for hiding this comment

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

Let's see how it goes in practice I guess?

"prompt": "^1.0.0"
},
"dependencies": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

wow, can't believe this was missed so long...

@@ -0,0 +1,48 @@
#! /usr/bin/env node
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this work if you don't have node installed globally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably not, but neither will anything else in our build process.

Copy link
Member

Choose a reason for hiding this comment

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

if it is for ci environment and not part of mvn command cycle, is ok as we use that elsewhere...

but I guess this does require devs to have npm/node installed (not unreasonable for development, as long as it isn't required for mvn commands right @kzantow?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now it's client-side only, so it only needs to run on dev machines.

@@ -0,0 +1,226 @@
#! /usr/bin/env node
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto

Copy link
Member

Choose a reason for hiding this comment

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

see above - is dev workstation only script and opt-in for now

@michaelneale
Copy link
Member

michaelneale commented Oct 22, 2017

@sophistifunk this has upset the eslint rule checks, so I guess they have to be removed in favour of prettier? (as they no longer apply?) (can see it in build results)

@sophistifunk
Copy link
Collaborator Author

@michaelneale the editor had a few semantic rules turned off that are on in the other areas, I'll revert that one for now.

@michaelneale
Copy link
Member

nice @sophistifunk

Lets see if master can get stable with either #1484 or #1503 - then perhaps merge it in if @cliffmeyers is ok?

@sophistifunk
Copy link
Collaborator Author

Green! Anybody with objections to merging, speak now or forever hold your peace ;-)

@kzantow
Copy link
Collaborator

kzantow commented Oct 23, 2017

160 :)

@sophistifunk
Copy link
Collaborator Author

Stop doing so much with each statement :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants