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

var => let/const #436

Closed
matteofigus opened this issue Apr 4, 2017 · 12 comments
Closed

var => let/const #436

matteofigus opened this issue Apr 4, 2017 · 12 comments

Comments

@matteofigus
Copy link
Member

A non critical optimisation that would be ideal for a first time contribution to OC.

Being very careful and perhaps splitting the work into multiple PRs to improve safety 🤣

@elboletaire
Copy link
Collaborator

elboletaire commented Apr 4, 2017

Eslint may help with that:

using the --fix option

@matteofigus
Copy link
Member Author

Yeah, actually @elboletaire you remind me that as part of this, we should probably move from jshint to eslint as it's much better for that!

@nickbalestra
Copy link
Contributor

@elboletaire
Copy link
Collaborator

@matteofigus moving to eslint would be a good move, since jshint seems a bit outdated for all the ES6 stuff. Adding eslint to the project is as easy as eslint --init, and specify the source folders after asking some questions.

In fact I've done it locally but, there's something that scares me a bit... all that double negations found in a lot of files. Are they really needed? If they're there for casting to bools, wouldn't it be better to use ES5's ToBoolean?

Obviously I can ignore the warnings just changing a rule, although I'm still curious about it.

PS. If you want I can do a PR with the eslint added respecting the current "rules", and then another to change all the var with const and let.

@matteofigus
Copy link
Member Author

@elboletaire the !!castings definitely need to be checked one by one, it may be an hard work. I think most of them are not necessary.

I would love a PR for eslint, but I would first like to ensure everybody wants it. @nickbalestra you linked prettier which seems a nice project but I don't know so much about it. Do you think switching to eslint is something we still want to do it independently from prettier, or are the two things mutually exclusive?

@mattiaerre
Copy link
Member

@matteofigus I am not a prettier expert either but I think that if we incrementally improve our coding style, an eslint first step will not stop us to move towards prettier later if we want to. I, on the other hand, use eslint a lot and I see all the benefits.

// cc @elboletaire @nickbalestra

@nickbalestra
Copy link
Contributor

nickbalestra commented Apr 5, 2017

Yes, you can use both in combinations (they complete each other and solve slightly different problems.). I would suggest a two fold process as well:

  1. switch to eslint
  2. add prettier to eslint.

Would ❤️ to see @elboletaire PR for eslint :)And love how he suggest to takle it into sub PRs (eslint first, then let, const, ..)

@matteofigus
Copy link
Member Author

Nice! So yeah if you want to make a PR go on, thanks @elboletaire !!

@elboletaire
Copy link
Collaborator

Sure, I'll try to do it during this week 😃

@nickbalestra
Copy link
Contributor

nickbalestra commented Apr 5, 2017

@elboletaire @matteofigus @mattiaerre If is not too urgent, I would like to investigate a bit more into prettier, before going eslint...

This issue was mainly to start using scoped let and constants instead of variables, and I wonder if for that we need eslint. If we go eslint we should also think of which rules to enable/disable, what were your thoughts regarding to eslint config @elboletaire ?

@elboletaire
Copy link
Collaborator

elboletaire commented Apr 5, 2017

Use the autodetect feature and fix any warnings adapting the rules to your current code style (if can't adapt them, just disable). That for the eslint file creation (first PR).

After that, change the rules that we want to switch to, like the cost/let, and fix the issues (many of them can be automatically fixed with --fix).

@nickbalestra
Copy link
Contributor

Great, my concern was not to start with too many rules or presets already, but I like your plan @elboletaire !

elboletaire added a commit to elboletaire/oc that referenced this issue Apr 7, 2017
elboletaire added a commit to elboletaire/oc that referenced this issue Apr 7, 2017
elboletaire added a commit to elboletaire/oc that referenced this issue Apr 7, 2017
elboletaire added a commit to elboletaire/oc that referenced this issue Apr 7, 2017
elboletaire added a commit to elboletaire/oc that referenced this issue Apr 7, 2017
elboletaire added a commit to elboletaire/oc that referenced this issue Apr 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants