Skip to content
This repository has been archived by the owner on Jul 27, 2018. It is now read-only.

Reformat code to follow standard #1242

Closed
Gozala opened this issue Aug 19, 2016 · 3 comments
Closed

Reformat code to follow standard #1242

Gozala opened this issue Aug 19, 2016 · 3 comments

Comments

@Gozala
Copy link
Contributor

Gozala commented Aug 19, 2016

Ok so once #1241 lands we'll be linting all new code to comply with standard style. Unfortunately automatically reformatting all of the existing code tracked by #1238 seems far from trivial task and still unclear if it's a worthy effort.

Alternative approach would be to just run a lint file by file and address errors programmatically or manually. As far as I can tell eslint --fix ./path/to/file.js seems to address majority of errors, but not the comma first issues (see eslint/eslint#6941).

This is also maybe an easy first contribution patch. In case you want to contribute to this issue you should:

  1. Remove path for the file you're going to fix from .eslintignore file.
  2. Run lint and address it's errors.
  3. Submit a change via pull request.

Note: This way we can gradually reformat our code.

P.S: No need to reformat stuff in src/About/Newtab/Demos/ as that code is not maintained in this repo.

@onurtemizkan
Copy link
Contributor

Hey @Gozala , I'm going to work on this. But just to be sure, you prefer one PR for each file, right? It would be a lot easier if I submit a PR for each directory of files. What do you think?

@Gozala
Copy link
Contributor Author

Gozala commented Sep 2, 2016

Hey @Gozala , I'm going to work on this.

Thanks that would be really great!

But just to be sure, you prefer one PR for each file, right?

If it's just formatting changes (as it should be) I don't mind multiple files or even all files in one PR.

It would be a lot easier if I submit a PR for each directory of files. What do you think?

Only reason I think file by file PRs would be better though is because that would reduce possibility of merge conflicts.

@magopian
Copy link
Contributor

magopian commented Feb 22, 2017

The command isn't eslint anymore, but standard-flow, and I believe the process is now as follows:

  • remove the file you want to work on from the standard-flow: { "ignore" : ...} list in the package.json file
  • run npm run lint (or node_modules/.bin/standard-flow --fix to have some of the warnings automatically fixed)
  • fix the (remaining) errors manually
  • finally run npm run test to make sure nothing's broken
  • rinse and repeat

A quick tip to easily move all the commas at the end of the previous line, if you're using vim:
:%s/\(\_s\{2,\}\),/,\1/g. Beware that this is a very naive approach, and it'll move a comma to the previous line even if it shouldn't move it further up (eg. if the previous line is a comment).

magopian added a commit to magopian/browserhtml that referenced this issue Feb 22, 2017
magopian added a commit to magopian/browserhtml that referenced this issue Feb 23, 2017
@Gozala Gozala closed this as completed in 7ae365b Apr 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants