Skip to content

Clean up javascript based on js2-mode feedback. #1036

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

Merged
merged 1 commit into from
Nov 27, 2011

Conversation

stefanv
Copy link
Contributor

@stefanv stefanv commented Nov 23, 2011

No description provided.

@minrk
Copy link
Member

minrk commented Nov 23, 2011

Do you know why it removes semicolons after some }, and adds them on others (all in codecell.js)?

As I understand it, semicolons are not functional under normal circumstances, unless you want to put multiple statements on a single line, and a few rare special cases. It would seem that humans come down in favor of superfluous semicolons about 60/40, but robots (jsflakes, etc.) seem to always recommend them.

@stefanv
Copy link
Contributor Author

stefanv commented Nov 24, 2011

js2-mode actually just highlights the missing semi-colons; I removed a couple of semi-colons after for-loops based on the output of jslint, but this seemed to be a waste of time (not that the rest of the exercise isn't :-).

@stefanv
Copy link
Contributor Author

stefanv commented Nov 24, 2011

I just assumed that js2 was correct, but I now see that there are many contentious discussions on Stackoverflow. What's the IPython preference regarding semi-colons?

@stefanv
Copy link
Contributor Author

stefanv commented Nov 24, 2011

Looking at this further, I can see why the style checker recommended these semi-colons. Most are variable assignments of the form:

var some_func = function () { ... };

@minrk
Copy link
Member

minrk commented Nov 24, 2011

I see - the ones that are added are on assignments of functions, which are technically vulnerable to subsequent closures (of which there are none). Semicolons seem to me to be almost as silly in js as they are in Python, but might as well be consistent.

We are probably going to have a lot of ignored semicolons as we get Python programmers to write more javascript :)

This seems perfectly fine to me, but might wait for PR #987 before merging, to avoid conflict since that one actually has code.

@minrk
Copy link
Member

minrk commented Nov 24, 2011

I think the IPython js style is probably to use semicolons, because that seems to be the vague javascript convention, but no need to pass every commit through jsflakes or anything, because they don't actually matter 99% of the time, and code-style tools are more cost than benefit.

@stefanv
Copy link
Contributor Author

stefanv commented Nov 24, 2011

Agreed; although, the closure after function definition issue may be worth being careful about.

@fperez
Copy link
Member

fperez commented Nov 24, 2011

I think we did have one or two actual bugs a while ago that were in fact caused by missing semicolons. I wish I remembered more precisely...

In any case, waiting on #987 seems like a good idea, @Carreau has put a lot of work into that code and we don't really want to create conflicts on that PR just for code cleanup.

@stefanv
Copy link
Contributor Author

stefanv commented Nov 24, 2011

Yup, that's fine!

@fperez
Copy link
Member

fperez commented Nov 24, 2011

@stefanv, it looks like this needs a rebase already (and will likely need another when #987 comes in, though I find it typically easier to sort out rebase conflicts in smaller, incremental chunks).

@fperez
Copy link
Member

fperez commented Nov 24, 2011

@stefanv, #987 was merged, so if you want to rebase this we can go ahead and merge it quickly.

@stefanv
Copy link
Contributor Author

stefanv commented Nov 24, 2011

I'm not near a computer atm, but can do it first thing tomorrow. If new js
was added, we may want to re-check the added files. Having a vim or emacs
plugin handy while editing javascript is probably a good idea (especially
for us Python programmers, as @minrk mentioned).
On Nov 24, 2011 1:40 PM, "Fernando Perez" <
reply@reply.github.com>
wrote:

@stefanv, #987 was merged, so if you want to rebase this we can go ahead
and merge it quickly.


Reply to this email directly or view it on GitHub:
#1036 (comment)

@fperez
Copy link
Member

fperez commented Nov 26, 2011

@stefanv, I agree that having a plugin handy to occasionally check js code would be a good idea. I don't like to run always under a code verifyer (they get annoying when you're in the middle of refactoring), but I do regularly check things with pyflakes. If you know of something similar for js in emacs let me know.

I can have a look at this one whenever you have a chance to rebase.

@stefanv
Copy link
Contributor Author

stefanv commented Nov 27, 2011

@fperez Rebased. I use js2-mode in Emacs, which does highlighting + syntax checking.

@fperez
Copy link
Member

fperez commented Nov 27, 2011

Thanks, merged!

fperez added a commit that referenced this pull request Nov 27, 2011
Clean up javascript based on js2-mode feedback.
@fperez fperez merged commit 005ffb0 into ipython:master Nov 27, 2011
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Clean up javascript based on js2-mode feedback.
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.

3 participants