Fixes #1242: reformat files to follow standard by magopian · Pull Request #1269 · browserhtml/browserhtml

magopian · 2017-02-23T10:56:05Z

This PR only deals with the easy fixes:

move commas to the end of the line
curly braces placement
remove unused imports and declaration
replace == and != by === and !== (when not comparing to null or undefined)
various straightforward small fixes

…ndard

…andard

….js and src/Browser/Navigators/Navigator/Assistant/Suggestion.js

…c/Common/Favicon.js

…ers from src/Common/URLHelper.js

…type parameters, namespace XMLHttpRequest with 'window' in src/Devtools/Record.js

…ttpRequest with 'window' in src/Devtools/Replay.js

…ement in src/Lang/Task.js

…e/fetch.js

…as for src/Common/Deck.js

…y braces for src/Common/Style.js

magopian · 2017-02-27T10:06:23Z

I believe I've been as far as I could with my knowledge of js, flow, standard and the codebase.

I'm not sure how to fix the 7 remaining files, and would welcome any tip or help on those.

Maybe dealing with those problematic ones in another PR would make sense though, this one is already pretty large.

@Gozala any thoughts?

Gozala · 2017-02-28T00:15:10Z

I believe I've been as far as I could with my knowledge of js, flow, standard and the codebase.

That is amazing! Thank you so much! I'll try to get through this by end of tomorrow!

I'm not sure how to fix the 7 remaining files, and would welcome any tip or help on those.

I'll definitely try to look into those, but let's get these landed first.

Maybe dealing with those problematic ones in another PR would make sense though, this one is already pretty large.

Most certainly! I would rather land formatting changes separate from ones that actually alter code, so that if we introduce some regressions it would be easier to track it down and fix.

Gozala

Thanks again for all the hard work! I can't wait to get this merged, there are just few things to be addressed.
Feel free to ignore few comments I did about the indentation, I am surprised that it still complies with standard. I am tempted to pass code through prettier before handing it over to standard (as discussed in prettier/prettier#736) just so that code style is more consistent but that's probably better done as followup.

So I think it's just a new String and some code removal and it should be ready to go.

Also my apologies for taking this long with review, it's just this change is huge.

Gozala · 2017-02-28T18:54:11Z

main.js

-    ? { flag
-      , query
+    ? { flag,
+       query


Indentation here is odd, not sure why lint is not catching it, but my understanding of the rules is it should be as:

{ flags, query }

I didn't see any indentation "errors" caught by the linter, so I don't think it's even checking it. I'll fix those indentations issues with pleasure ;)

Gozala · 2017-02-28T18:55:32Z

main.js

-    , query: ""
+    ),
+   { flag: null,
+     query: ''


Same as in comment above

Gozala · 2017-02-28T18:56:05Z

main.js

-    , webPreferences:
-      { nodeIntegration: true
-      , preload: path.resolve(path.join('.'), 'electron-preload.js')
+  mainWindow = new BrowserWindow({ width: 1024,


I think width: 1024 should be on next line.

Gozala · 2017-02-28T18:56:35Z

main.js

+     height: 740,
+     frame: false,
+     webPreferences:
+      { nodeIntegration: true,


I think it should be webPreferences: {\n

Gozala · 2017-02-28T18:59:15Z

src/About/Repl/Repl/Cell.js

-  { id: ID
-  , input: Input.Model
-  , output: Output.Model
+  { id: ID,


I think it objects keys should start on a line after { according to standard format.

Gozala · 2017-02-28T19:24:22Z

src/About/Repl/Repl/Host/Window.js

-
-
-const DELETE = new String('delete');
+const DELETE = 'delete'


It is important for DELETE to be a special unexposed object rather than plain string which is what new String('delete') produced as it's treated as special instruction.

I'll change it back, but then i'll leave this file in the standard-flow ignore list, as it complains:

standard-flow: Use JavaScript Standard Style (with Flow types) (https://github.com/gozala/standard-flow) /Users/mathieu/mozilla/browserhtml/src/About/Repl/Repl/Host/Window.js:14:16: Do not use String as a constructor. /Users/mathieu/mozilla/browserhtml/src/About/Repl/Repl/Host/Window.js:69:57: eval can be harmful.```

Gozala · 2017-03-02T00:09:55Z

src/Browser/Navigators/Navigator/Assistant/Suggestion.js

    }
  }

-export const update =


Why is this function gone now ?

Can you please keep this function, even if it's not used right now. I would rather remove obsolete code in separate change from formatting one.

Gozala · 2017-03-02T08:04:48Z

src/Devtools/Log.js

-
-
-export type Model <model, action> =
+export type Model <model, action> = // eslint-disable-line no-unused-vars


Question: Why eslint-disable-line no-unused-vars is necessary here ? exports aren't considered unused are they ?

It's the model type that is unused, and that's why it's complaining.

Gozala

Can you please undo deletes that I've pointed out so we can land this ?

Gozala · 2017-03-03T00:39:32Z

src/Browser/Navigators/Navigator/Assistant.js


-
-
-export const updateSuggestion =


Can you please keep this function, I know it's not used but it's going to be soon. And in any case I'd rather prune obsolete code in separate change.

Gozala · 2017-03-03T00:41:49Z

src/Browser/Navigators/Navigator/Assistant/Suggestion.js

    }
  }

-export const update =


Can you please keep this function, even if it's not used right now. I would rather remove obsolete code in separate change from formatting one.

Gozala · 2017-03-03T00:48:47Z

src/Common/Animation.js

-    (Task.requestAnimationFrame().map(Tick))
-  ]
-
-const endTransition = <model>


Please export this function instead of deleting it.

magopian · 2017-03-08T09:10:43Z

I believe I've addressed all your comments, please let me know if I missed something or if you'd need anything else, i'd be happy to help!

magopian added 17 commits February 22, 2017 16:21

Fixes browserhtml#1242: first batch of reformated files to follow sta…

c530609

…ndard

Fixes browserhtml#1242: second batch of reformated files to follow st…

9b023c5

…andard

Drop redundant css from src/About/Settings/Settings.js

77c0c6a

Remove some dead code from src/Browser/Navigators/Navigator/Assistant…

760814d

….js and src/Browser/Navigators/Navigator/Assistant/Suggestion.js

Remove an unused type parameter from src/Common/Animation.js

cc86bae

Move some commas to the end and replace != by !== and == by === in sr…

2fc07a6

…c/Common/Favicon.js

Remove unused 'kind' type from src/Common/Keyboard.js

ec6443e

Fix curly braces placement in src/Common/Prelude.js

bf05192

Remove unnecessarily computed property from src/Common/URL.js

4b63376

change location to window.location, remove unnecessary escape charact…

828b0d8

…ers from src/Common/URLHelper.js

Remove some unused type parameters from src/Devtools.js

6166d8d

Ignore unused type parameters in src/Devtools/Log.js

015b555

Ignore unused type parameters, ignore eslint warning for some needed …

2f23be8

…type parameters, namespace XMLHttpRequest with 'window' in src/Devtools/Record.js

Ignore eslint warning for some needed type parameters, namespace XMLH…

0eef890

…ttpRequest with 'window' in src/Devtools/Replay.js

no assignment in return statement, ignore unsafe usage of return stat…

49f41a1

…ement in src/Lang/Task.js

Namespace XMLHttpRequest with 'window' in src/Service/Search.js

d766506

Titlecase the 'short' type, surround @@iterator by quotes in interfac…

dbad640

…e/fetch.js

magopian force-pushed the 1242-reformat-code-to-follow-standard branch from 33ffa90 to 5809efc Compare February 27, 2017 09:28

Rewrite type parameters to avoid standard-flow parser error, fix comm…

62c1a6e

…as for src/Common/Deck.js

magopian force-pushed the 1242-reformat-code-to-follow-standard branch from 5809efc to 62c1a6e Compare February 27, 2017 10:02

Rewrite type parameters to avoid standard-flow parser error, fix curl…

ddb71d6

…y braces for src/Common/Style.js

Gozala suggested changes Mar 2, 2017

View reviewed changes

magopian added 2 commits March 2, 2017 09:46

Fix indentation issues following @Gozala's review

ec7920e

new String() is needed as per @Gozala's review

9fea9ec

Gozala self-assigned this Mar 3, 2017

Gozala reviewed Mar 3, 2017

View reviewed changes

Gozala mentioned this pull request Mar 3, 2017

Integrate prettier into toolchain to deal with code formatting. #1271

Closed

re-add maybe obsolete code

283bcac

magopian force-pushed the 1242-reformat-code-to-follow-standard branch from e0c7cda to 283bcac Compare March 8, 2017 09:08

Gozala approved these changes Apr 5, 2017

View reviewed changes

Gozala merged commit 7ae365b into browserhtml:master Apr 5, 2017



		export type Model <model, action> =
		export type Model <model, action> = // eslint-disable-line no-unused-vars

Conversation

magopian commented Feb 23, 2017 • edited by Gozala Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magopian commented Feb 27, 2017

Uh oh!

Gozala commented Feb 28, 2017

Uh oh!

Gozala left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gozala left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magopian commented Mar 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

magopian commented Feb 23, 2017 •

edited by Gozala

Loading