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

Clean code base #61

Open
hwaterke opened this issue May 26, 2017 · 4 comments
Open

Clean code base #61

hwaterke opened this issue May 26, 2017 · 4 comments

Comments

@hwaterke
Copy link
Contributor

Hey,

This is probably a bit of a strange request but while working on my last PR on this project a few things triggered my OCD.

For example:

  • var is used everywhere instead of const / let
  • unused imports in many files
  • unconsistent use of single/double quotes
  • etc.

It is not crucial but would you welcome a PR for this ? :P
Have you ever considered using prettier to format the code in a consistent way?

I was also wondering why you commit the build on Github? (dist folder)

Let me know what you think!

H.

@sporto
Copy link
Contributor

sporto commented May 26, 2017

Sure, it will would be great to have a PR to clean the code.

  • re var. I don't really see the benefits of using const or let tbh. Can't remember a bug because of var. But ok.
  • removing unused import would be great.
  • re quotes, consistency would be great. I prefer double.
  • re prettier, I considered several times but because of this issue Respect line breaks in object literals prettier/prettier#66 I have never adopt it, as this project gets a few PRs I will be happy to use prettier and get over that
  • re dist. I think we need to provide that for people using the lib, otherwise they need to integrate a typescript compiler in their workflow

@hwaterke
Copy link
Contributor Author

hwaterke commented May 26, 2017

Alright cool, I'm gonna start working on it then.

To elaborate a bit on your points:

  • var has a so called function scope which is unintuitive to many developers. This easily leads to bugs for unexperienced developers. let and const have block scopes which is more common in other languages and therefore used properly by most developers.
  • double quotes then, doesn't change much for me as long as it is consistent
  • You have very relevant points in your issue on prettier and I would definitely configure prettier to format like that if it was possible. IMHO I would be ready to make the sacrifice in favour of consistent formatting but it's your call. Just tell me!
  • the dist folder definitely needs to be shipped when publishing on npm but I would not have it under source control. It adds noise to PRs and increases the size of the code base for no real benefit. Ideally the package.json is configured to build before pushing to npm and the dist folder is in your .gitignore. I can work on that too if you want. Also, if someone send a PR but forgot to build first, dist and src will be out of sync.

@sporto
Copy link
Contributor

sporto commented May 26, 2017

Cool. Yes to all.

Re prettier, I'm happy to use it as consistent formatting is quite important and removes discussions about it.

The workflow around dist would be great to have. Agree that PRs are terribly noisy because of this.

Thanks

@hwaterke
Copy link
Contributor Author

I just submitted #62 as a first step.
The whole code went through prettier. I'll wait for this to be merged before moving on the the next step in order to avoid awful merge conflicts.

I'll probably tackle the build script next to get rid of the dist folder asap.

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

2 participants