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 javascript formatting. #4436

Merged
merged 8 commits into from Oct 17, 2017

Conversation

Projects
None yet
5 participants
@dannon
Member

dannon commented Aug 16, 2017

Prettier formatting, extracted from #4435 for separate PR per @jmchilton's request.

Merge after #4435.

@dannon dannon added the status/WIP label Aug 16, 2017

@dannon dannon changed the title from Prettier javascript formatting. to [WIP] Prettier javascript formatting. Aug 16, 2017

@dannon dannon added the area/UI-UX label Aug 16, 2017

@dannon

This comment has been minimized.

Member

dannon commented Aug 16, 2017

There was conversation in #galaxyproject about whether or not we want to go with a 4-space or 2-space tab stop, with @martenson and @jxtx primarily. I think it was agreed that we need to have a standard and enforce it, but it seems like there are arguments for both 2 and 4. This PR currently enforces a 4-space tab standard, following the trend of much of our existing JS style, and our Python code style. That's easily removed by stripping 0669e78, but I think that what is presented here is a good starting point.

The best news is that with Prettify it's trivial to swap, should we change our mind down the road.

@willdurand

This comment has been minimized.

willdurand commented Aug 16, 2017

Let's go for 4 spaces. It is best to set the rule for the whole project so Python, CSS, JS should be all 4 spaces.

@martenson

This comment has been minimized.

Member

martenson commented Aug 16, 2017

converting all JS to 4 spaces 👍

@jmchilton

This comment has been minimized.

Member

jmchilton commented Aug 17, 2017

What I was requesting was a clean PR that just modified the code formatting - maybe after the build pipeline was in place (?) - so maybe a PR to setup the new build system and then a second afterward that actually builds the artifacts and modifies the formatting of the files. This still seems to mix Makefile changes, babel configuration, gulpfile addition, etc... in with the formatting changes. Github won't even load the list of all changed files for me. Like I said before though - it isn't feasible that is fine - it'd just help me grok the changes.

@dannon

This comment has been minimized.

Member

dannon commented Aug 17, 2017

@jmchilton This is branched off the other PR. When that's merged, all you'll see are the prettier addition and formatting changes. (and I'd like that merged first and this building on that, anyway)

@jmchilton

This comment has been minimized.

Member

jmchilton commented Aug 17, 2017

@dannon Ahhh... I see what you are saying - thanks for the clarification.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Aug 17, 2017

@dannon Indeed I see it now over in the other PR - it is a lot cleaner now - much easier to understand what is going on. Thanks for doing this!

@dannon

This comment has been minimized.

Member

dannon commented Aug 17, 2017

@jmchilton No problem, wasn't a big deal.

@dannon dannon added status/review and removed status/WIP labels Oct 12, 2017

@dannon

This comment has been minimized.

Member

dannon commented Oct 12, 2017

(rebased on top of #4435, cleaned up)

@dannon dannon changed the title from [WIP] Prettier javascript formatting. to Prettier javascript formatting. Oct 17, 2017

@dannon dannon removed the status/WIP label Oct 17, 2017

@galaxybot galaxybot added this to the 18.01 milestone Oct 17, 2017

@jmchilton jmchilton merged commit ee39e61 into galaxyproject:dev Oct 17, 2017

6 of 7 checks passed

toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
api test Build finished. 304 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 162 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 57 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript 7 new alerts and 7 fixed alerts
Details
selenium test Build finished. 98 tests run, 1 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment