5 ms·

There is one very important tip that's missing: Follow the original coding style exactly. Not just spaces vs tabs or block styles, but idioms and other idiosyn

by csl 10y ago

There is one very important tip that's missing: Follow the original coding style exactly. Not just spaces vs tabs or block styles, but idioms and other idiosyncrasies, too. Why? Imagine reading a source repo where every second block uses different bracket styles, mixing spaces with tabs and so on. It's going to look like a kludgy mess, and will be distracting to read. There is no correct style for most languages (perhaps `go fmt` might be an exception), only opinions.

hzoo 10y ago
Right! And a lot of projects have a linter in place that runs in continuous integration (travis, etc) that you can see in the PR or just locally. In Babel we use ESLint for this https://github.com/babel/babel/blob/master/Makefile#L20-L27 https://github.com/babel/babel/blob/master/Makefile#L20-L27
danso 10y ago
I agree, and it's one of the reasons why novice-to-intermediate programmers should try to pitch in to a project, even for something very minor. I remember making a pull request to a Ruby project and getting rejected and being told to fix all the rubocop errors, which made me aware of the existence of tools for auto-style-detection/linting, and of best practices in style that greatly improved my programming experience. That kind of practical thing is not well-covered in tutorials and self-learning curriculums.
sctblol 10y ago
flake8 for Python is nice, although defaults can be a bit weird...
- tedmiston 10y ago
  Code Climate or Codacy are nice too. They collect tools like flake8 and run a server side report kind of like coverage.
the8472 10y ago
> Follow the original coding style exactly. Personally I would take any formatting and just run an auto-formatter over the code section when I work on it the next time in case it bothers me. Correct and sane code are far more important than hassling someone else to conform to a particular style. In my opinion applying styles is a task for machines, not humans.
- Arnt 10y ago
  It's more than that. I've had a patch rejected because my unit tests used the wrong verification manner (java assertTrue() instead of assertThat() in cases where the latter would report better error messages were the test to fail, as I recall). Automatic blah will never catch this kind of thing. Ditto for human-readable messages that have/lack full stops at the end. Ditto for if() foo vs if() { foo } and the ever-lovely style question, are abbreviations such as TCP to be called TCP or Tcp in camelcased identifiers? Style is largely a human matter.
  - the8472 10y ago
    > Ditto for if() foo vs if() { foo } At least that one can be fixed automatically by eclipse. But sure, there are some things that are difficult to automate. What I'm saying that one should not hassle a contributor over all those things that that can be done by machines.
    - vsl 10y ago
      How is that "hassle"? Somebody needs to fix it, why should it be the maintainer? In my experience, not even bothering with style is a good indicator of careless "shotgun coding" and such PRs typically have far worse issues. It's also typical that such PRs won't have real issues fixed either.
      - the8472 10y ago
        > why should it be the maintainer? I'm saying: Why should it be either human? > In my experience, [...] That seems fairly dismissive to me. An expert in a certain area can certainly fix smaller issues with a few lines of code but only have a limited amount of time on their hands. Having to read a lengthy style guide and making sure they adhere to every single rule, especially when those rules are the opposite of their regular habits, is a non-zero hurdle to contributing. Offloading that work to a machine as far as possible lowers the burden to only those rules that cannot be applied automatically.

by csl 10y ago

hzoo 10y ago

Right! And a lot of projects have a linter in place that runs in continuous integration (travis, etc) that you can see in the PR or just locally. In Babel we use ESLint for this https://github.com/babel/babel/blob/master/Makefile#L20-L27 https://github.com/babel/babel/blob/master/Makefile#L20-L27

danso 10y ago

I agree, and it's one of the reasons why novice-to-intermediate programmers should try to pitch in to a project, even for something very minor. I remember making a pull request to a Ruby project and getting rejected and being told to fix all the rubocop errors, which made me aware of the existence of tools for auto-style-detection/linting, and of best practices in style that greatly improved my programming experience. That kind of practical thing is not well-covered in tutorials and self-learning curriculums.

sctblol 10y ago

flake8 for Python is nice, although defaults can be a bit weird...

tedmiston 10y ago
Code Climate or Codacy are nice too. They collect tools like flake8 and run a server side report kind of like coverage.

the8472 10y ago

> Follow the original coding style exactly. Personally I would take any formatting and just run an auto-formatter over the code section when I work on it the next time in case it bothers me. Correct and sane code are far more important than hassling someone else to conform to a particular style. In my opinion applying styles is a task for machines, not humans.

Arnt 10y ago
It's more than that. I've had a patch rejected because my unit tests used the wrong verification manner (java assertTrue() instead of assertThat() in cases where the latter would report better error messages were the test to fail, as I recall). Automatic blah will never catch this kind of thing. Ditto for human-readable messages that have/lack full stops at the end. Ditto for if() foo vs if() { foo } and the ever-lovely style question, are abbreviations such as TCP to be called TCP or Tcp in camelcased identifiers? Style is largely a human matter.
- the8472 10y ago
  > Ditto for if() foo vs if() { foo } At least that one can be fixed automatically by eclipse. But sure, there are some things that are difficult to automate. What I'm saying that one should not hassle a contributor over all those things that that can be done by machines.
  - vsl 10y ago
    How is that "hassle"? Somebody needs to fix it, why should it be the maintainer? In my experience, not even bothering with style is a good indicator of careless "shotgun coding" and such PRs typically have far worse issues. It's also typical that such PRs won't have real issues fixed either.
    - the8472 10y ago
      > why should it be the maintainer? I'm saying: Why should it be either human? > In my experience, [...] That seems fairly dismissive to me. An expert in a certain area can certainly fix smaller issues with a few lines of code but only have a limited amount of time on their hands. Having to read a lengthy style guide and making sure they adhere to every single rule, especially when those rules are the opposite of their regular habits, is a non-zero hurdle to contributing. Offloading that work to a machine as far as possible lowers the burden to only those rules that cannot be applied automatically.