Skip to content
This repository has been archived by the owner on May 2, 2019. It is now read-only.

Problem: the project has no defined style guide #225

Open
porcellus opened this issue Oct 16, 2018 · 8 comments
Open

Problem: the project has no defined style guide #225

porcellus opened this issue Oct 16, 2018 · 8 comments

Comments

@porcellus
Copy link
Contributor

This causes differences even on very basic things like tab width between files. (e.g.: import/modules/helpers.js - import/modules/notifier.js)
Solution: We should adopt the one from another project (like blockrazor) or the standard meteor style guide and enforce it through running eslint in CI

@gsovereignty
Copy link
Member

There's one on Blockrazor, we should just adopt that. The key thing when it comes to mixing style guides and C4 is to keep the style guide very minimal, only adding things that really create problems if it's not followed.

Linting is frowned upon because it hides code smell which we kind of want to see... umm... smell.

@anbud
Copy link
Member

anbud commented Oct 16, 2018

I agree, different indentations can be quite annoying, but they're not a functional issue. We should adopt Blockrazor's minimal style guide and that should be sufficient.

There was a discussion about this some time ago on Blockrazor, and it was decided that linting would probably cause more troubles. It hides code smells, as @gazhayes said, and it would probably cause lots of failing CI tests. And failing CI tests are annoying as you can't merge PRs with failing CI tests from a mobile device. On the other hand, if we just implemented a linter to clean the code on every PR, it would be against C4 as it involves direct writing to the repo.

@MadanBhandari
Copy link
Member

What about creating new repo for style guide, where we will maintain at one place and link that repo every Emurgo and Blockrazor projects for style guide instead o/f copying same in every repo.

@porcellus
Copy link
Contributor Author

I'm fine with what's in blockrazor. As for creating a separate repo for it: we could fork the Airbnb guide as that's what it is essentially. This is the best option in my opinion.

On the other issues: I think indentations can easily cause errors during coding and that in general having no enforced style guide slows down progress in the long run. I thought the point of linting was that it highlighted most common code smells. Besides, why merge PRs with failing CI, as that is forbidden by C4?
I don't really see how they hide code smells, could you link me to that discussion?

I'm strongly against the linter - or any automation - modifying the code when the PR lands.

In the end, I don't mind it either way, but I find it easier to pick up work after someone else if there is a common set of rules we work by, but I guess I just got too used to working in a tslint/prettier environment.

@porcellus
Copy link
Contributor Author

Any update?

@gsovereignty
Copy link
Member

I just looked at the Blockrazor style guide, I'm not sure how it got there but it's not a C4 style guide. To work with the C4 it has to be extremely simple, if you can't remember it in the morning before coffee it's too complicated.

I'll claim this and add a basic style guide and provide some guidelines on how to add stuff to it.

@gsovereignty
Copy link
Member

@emurgobot claim

@emurgobot
Copy link
Member

emurgobot commented Oct 31, 2018

Hello @gazhayes, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 3 days.

You can reclaim this issue or claim any other issue by commenting @emurgobot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants