-
Notifications
You must be signed in to change notification settings - Fork 610
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
Lets talk about code style... #289
Comments
I like ESlint recomends as basis and later from there start adapting the El 11/8/2016 6:26, "Tyler James Leonhardt" notifications@github.com
|
I think that's a great plan.
Not the biggest fan :P but I don't contribute anywhere near what you do so I can live with it :)
Love it. I'm finishing my first week here at Microsoft 😀 so I'm a tad busy ramping up... but I'd love to add this to the npm scripts |
Ok, let's start this way. If you want add it but don't make checks
Lol :-P
Fine :-)
Interm, or a permanent role? I had an interview for Skype some weeks ago |
I'm pretty sure they can all be set to warnings so that it doesn't interfere with any build processes or whatever.
Permanent role! let me see what I can do :) if you wanna email me your recruiter's name, I might be able to message them and see what happened! |
I was asking to don't exec them by default, but this is a better option, it can work as a reminder that they should be reviewed :-)
Ok! I'll send you an email :-) |
Hi everyone! Answer these questions for me: ? Are you using ES6 modules? Yes/No |
? Are you using ES6 modules? Yes/No
No, we use require(), but need to get used to them
? What style of indentation do you use? Spaces/Tabs
2 spaces
? What quotes do you use for strings? Single/Double
Single quotes
? What line endings do you use? Unix/Windows
Unix, of course!
? Do you require semicolons? Yes/No
No
? What format do you want your config file to be in? JSON
Obviously JSON :-D
|
So you think I should say 'yes' so that we can use them in the future? |
I'm not sure... I find require() easier to use, but I know es6 modules are Does any browser already support es6 imports? I think it would be a good El 13/8/2016 19:04, "Tyler James Leonhardt" notifications@github.com
|
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import well there's our answer |
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import Note: This feature is not implemented in any browsers natively at this Definitely require(), period. Import is only supported on dev builds of |
Full questions: ? Are you using ECMAScript 6 features? Yes |
? Where will your code run? Node (just Node, right? Not Node and Browser?)
Yes, only browser. Maybe in the (far) future there would be a port to run
it inside browsers, but it's not on the roadmap. Don't expect it before
NodeOS 3.0... ;-)
|
So I've got the eslintrc.json present and when I open the code up in my favorite text editor, boom. I get a whole bunch of stuff saying "do it this way" "do it that way" it's great! I imagine I'll have to add this to every repo in NodeOS? The next big part is figuring out where this goes in the build process, if at all... any thoughts? Of course the plan is to show warnings only for now. It could be just another package.json command that you run separately from any building steps... or it could be integrated into it somehow. |
That, or create a module similar to 'standard' (nodeos-standard? That would
Maybe as the last step of the test process? |
This is an interesting idea. Off the top of my head I would think that nodeos-standard would have a dependency of ESLint and then, while npm installing, nodeos-standard it would either point ESLint to an .eslintrc.json file that's in nodeos-standard's repo or it would throw an .eslintrc.json into the root of that repo that installed it. |
Easier than that, just fork 'standard' repo and modify it :-D El 13/8/2016 21:59, "Tyler James Leonhardt" notifications@github.com
|
I need to fix the readme and tests and whatnot I guess... haha but it's a start: |
Cool! Let's see how we optimize it ;-) El 14/8/2016 0:10, "Tyler James Leonhardt" notifications@github.com
|
https://www.npmjs.com/package/nodeos-standard the npm link! Next step, setting everything to warnings... lol |
Next step, setting everything to warnings... lol
Ideally not... if possible I would left the rules somewhat restrictive and
instead configure eslint to override the rules to be warnings.
|
Yeah like... If ESLint was going to say error, make it instead say warning On Sat, Aug 13, 2016, 3:37 PM Jesús Leganés Combarro <
|
Yes :-) Later when we are sure and everything mostly working we can remove El 14/8/2016 0:49, "Tyler James Leonhardt" notifications@github.com
|
I haven't found a good solution to this yet... You can easily ignore files... but besides that, suppressing errors to warnings with a global flag doesn't seem to exist... Maybe it's best to just ignore files we don't want to fix at the moment? |
In other news... here are the issues with the main repo if you're curious ;)
|
Those indentations ;) |
You are right, there's an option to supress warnings but not to convert
Ignore files is mostly for third party files, ignore your own errors is |
Those indentations ;)
I think it's a bad defined rule, probably related to the Allman style or
something similar. I will try to review it later.
|
I love the name of the flag X-D |
@piranna: I added allman just for you :P https://github.com/tylerl0706/nodeos-standard/blob/master/eslintrc.json Also, I added |
;-)
What do you mean? Are you talking about the errors vs. warnings thing? Unrelated: how we can make Atom to detect the nodeos-standard module and |
Just configure Atom's linter to use our .eslint ?! |
Don't know, maybe... El 18/8/2016 23:07, "John Green" notifications@github.com escribió:
|
Yeah so now, ESLint will still throw errors and warnings... but it wont crash. So we can still see our ugly code style, but the project will still run. That's what the |
#294 PR up :) |
This is totally doable. The path is:
In your favorite text editor, you should be able to configure this. I'm looking into forking this for a little bit better of an experience than manual configuration: |
Would it be better to have a style checker command, a eslint file on all the projects, or using both at the same time? |
Well... Nodeos-standard is basically just an eslintrc file that you install with a command rather than copy and pasting. And with ESLint, if you run "eslint file.js" it will lint that file based on the eslintrc file that's in your current directory. If we just stored the ESLintrc in the root, we would automatically get text editor plugins because ESLint is so widely used. If we go the "standard way" and have a nodeos-standard package we have to make all the text editor plugins... Which is basically just a fork of standard plugins... Usually people use both text editor plugins and commands because the text editor plugins will be preventative and the commands will be kind of a preventative measure for people who don't use the plugins. Lol that was a lot... It's a weird thing to explain |
Or... we can add a |
@piranna love that idea... how can I run a script when it installs? |
Just add an "install" entry on the "scripts" field of package.json file El 23/8/2016 7:39, "Tyler James Leonhardt" notifications@github.com
|
I know NodeOS is only working on Linux at the moment... But will this plan On Mon, Aug 22, 2016, 11:19 PM Jesús Leganés Combarro <
|
I know NodeOS is only working on Linux at the moment... But will this plan
work for all platforms?
Yes, script entries are shell commands, but in case of doubt, is possible
to set it to point to a Node.js script.
|
@piranna See nodeos-standard's latest push :D (TylerLeonhardt/nodeos-standard@a3baf0a) It will now create a symlink on install. |
Yay! It works! :-D |
So we want to have some sort of standard so that our code moving forward is clean and uses "best practices"
ESLint seems to be the best tool for this job... now it's just a matter of picking a preferred style. So far (based on what we've talked about) here are our options:
There are other options here: http://jscs.info/overview via jscs which has merged with ESLint.
Since ESLint is so flexible, any rules that we don't like we can change or disable.
Discuss!
The text was updated successfully, but these errors were encountered: