opened
larshp
username_0
fix comma so it follows same convention as the rest of the file

*************************

Please consider the following before submitting a pull request:

Guidelines for contributing: https://github.com/chartjs/Chart.js/blob/master/docs/developers/contributing.md

Example of changes on an interactive website such as the following:
- http://jsbin.com/
- http://jsfiddle.net/
- http://codepen.io/pen/
- Premade template: http://codepen.io/pen?template=JXVYzq
samples: fix convention, stacked.html
issue
created
simonbrunel
username_1
Thanks @username_0. It's not part of the guidelines since it's not enforced by ESLint. This is to prevent this kind of (IMO noisy) history, so if we merge this PR (and followings), we should turn the ESLint rule on and make only one commit with all extra comma removed.

But we already had this discussion and decided to allow dangling comma (see #3308 and this [comment](/chartjs/Chart.js/pull/3201#issuecomment-246359907)), so I think I would prefer not to merge this change.

@username_2 @username_3 thoughts?
comment
created
etimberg
username_2
@username_1 I had forgotten about the discussion in #3308 regarding the samples. If there aren't a lot of errors to fix then turning on eslint for the samples might be a good idea
comment
created
benmccann
username_3
Running `gulp lint` runs it on the samples and returns only a single samples-related warning. Codeclimate is not run on the samples, but it seems like it would be a good idea to turn it on. PR here: https://github.com/chartjs/Chart.js/pull/5092
comment
created
simonbrunel
username_1
`gulp lint` should include tests and samples, not CodeClimate.
comment
created
simonbrunel
username_1
@username_2 we already [run eslint against tests and samples](/chartjs/Chart.js/blob/master/gulpfile.js#L129), but we decided to allow dangling comma in lists and objects to prevent history noise (which is what happen with that PR). If we merge this one, that means we want to enforce no dangling comma, and I'm sure it's a good idea base on past discussions.

If we finally agree to change our mind, then we should enable the [ESLint rule](https://eslint.org/docs/rules/comma-dangle) to avoid punctual PRs for these useless changes. We can also enforce the other way, ie always have dangling comma, but IMO, better to close this PR and keep flexibility with that rule.
comment
created
etimberg
username_2
Ah, ok. I was thinking that the samples weren't linted. In that case, I don't think we should merge this for the reasons @username_1 outlined above
comment
created
benmccann
username_3
Fine with me. I don't care one way or the other about this. Though I do think that we should make lint and code climate consistent with each other in regards to which directories are covered
comment
created
simonbrunel
username_1
@username_3 that's what I initially thought, but code climate does extra checks that are not relevant for tests and samples, for instance expectations in unit tests are reported as duplicated code. So finally, it doesn't make sense to have code climate parsing the same files as ESLint because the reported score will be degraded for something we can't or don't want to change.
comment