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

Codestyle PRs #60

Closed
tinovyatkin opened this issue Jul 25, 2017 · 9 comments
Closed

Codestyle PRs #60

tinovyatkin opened this issue Jul 25, 2017 · 9 comments

Comments

@tinovyatkin
Copy link
Contributor

tinovyatkin commented Jul 25, 2017

I just took a look for the library source code and wondering why the code-style is so strange, mixed and outdated. Your node version requirement in package.json is current LTS (6.0) and that's great, but platform/language features usage seems to be targeting something like node 0.12.

For example: https://github.com/XeroAPI/xero-node/blob/master/lib/application.js - you have const within function declarations, but missing use strict anywhere and top level variables declared as var.

Question is: will you accept refactoring pull requests? I'm particularly interesting in getting rid of lodash as it's huge stone age dependency...

@philals
Copy link
Contributor

philals commented Jul 25, 2017

What about a road map on the README.md? Then contributors can pick up tasks.

Ideas to start with:

  • Add ESLint rules
  • ESLint rule complaint
  • Remove log4js
  • Throw exceptions with messages
  • Remove lodash

I can open a PR with these changes we think it's a good idea.

@jordanwalsh23
Copy link
Contributor

jordanwalsh23 commented Jul 25, 2017 via email

@philals
Copy link
Contributor

philals commented Jul 26, 2017

Here we go @jordanwalsh23 and @tinovyatkin

#61 Adds a quick road map, which hopefully will encourage the community to PR on the old code.

@tinovyatkin
Copy link
Contributor Author

Great! I think ESLint is essential for productive collaboration, so, will start with that small PR.

@philals
Copy link
Contributor

philals commented Jul 26, 2017

Nice @tinovyatkin. I think ESLint will give the greatest benefit.

Thanks.

I guess you get to choose the style to go with.

@tinovyatkin
Copy link
Contributor Author

Well, I have a great experience with airbnb rules set + eslint-prettier, but whatever you prefer it's more important for me to have it automatically enforced in the project via tooling.

@philals
Copy link
Contributor

philals commented Jul 26, 2017

I agree with you. Does not matter what it is, as long as it's there and automated.

airbnb and eslint-prettier will be great.

@tinovyatkin
Copy link
Contributor Author

Submitted #63

@jordanwalsh23
Copy link
Contributor

This has been merged. Closing the issue.

jordanwalsh23 added a commit that referenced this issue Aug 25, 2017
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

No branches or pull requests

3 participants