Issue 29346533: Issue 3693 - Created global editorconfig (Closed)
|
Created:
June 15, 2016, 1:16 p.m. by juliandoucette Modified:
Sept. 27, 2016, 7:16 p.m. CC:
Sebastian Noack, martin Visibility:
Public. |
DescriptionCreated global editorconfig
Repository: hg.adblockplus.org/codingtools/
Patch Set 1 #
Total comments: 6
MessagesTotal messages: 19
Updated reviewers based on discussion in last Frontend Developers meeting.
LGTM
LGTM
Otherwise LGTM https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig File .editorconfig (right): https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig#newcode6 .editorconfig:6: # Defaults Perhaps we should add `max_line_length = 80`?
https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig File .editorconfig (right): https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig#newcode6 .editorconfig:6: # Defaults On 2016/09/16 10:01:05, kzar wrote: > Perhaps we should add `max_line_length = 80`? "Supported By A Limited Number of Editors" But even if it were supported widely, I don't really see how hard wrapping would be useful.
https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig File .editorconfig (right): https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig#newcode6 .editorconfig:6: # Defaults On 2016/09/16 10:29:01, Wladimir Palant wrote: > On 2016/09/16 10:01:05, kzar wrote: > > Perhaps we should add `max_line_length = 80`? > > "Supported By A Limited Number of Editors" > > But even if it were supported widely, I don't really see how hard wrapping would > be useful. Ah you're right, never mind.
https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig File .editorconfig (right): https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig#newcode6 .editorconfig:6: # Defaults On 2016/09/16 10:29:01, Wladimir Palant wrote: > On 2016/09/16 10:01:05, kzar wrote: > > Perhaps we should add `max_line_length = 80`? > > "Supported By A Limited Number of Editors" > > But even if it were supported widely, I don't really see how hard wrapping would > be useful. According to https://github.com/editorconfig/editorconfig/wiki/Property-research:-maximum-... the behavior of this is inconsistent anyway. In Sublime Text and Vim this is merely adding an indicator (which is also why I'm using it) whereas in Emacs it wraps the lines automatically. Having an indicator is still quite useful though. https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig#newco... .editorconfig:11: trim_trailing_whitespace = true For me personally, this one is quite inconvenient because I save my files quite often while editing. Therefore it would cause my cursor to jump to the beginning of the line if I had this setting enabled. I assume that I'm probably the only one with that issue and that the config can still be overridden but just wanted to note it.
https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig File .editorconfig (right): https://codereview.adblockplus.org/29346533/diff/29346534/.editorconfig#newco... .editorconfig:11: trim_trailing_whitespace = true On 2016/09/16 11:55:54, Thomas Greiner wrote: > For me personally, this one is quite inconvenient because I save my files quite > often while editing. Therefore it would cause my cursor to jump to the beginning > of the line if I had this setting enabled. I guess that's why Atom isn't trimming whitespace on the current line - for people like you ;)
LGTM
LGTM
I don't have permission to push to this repository. Can someone please: - Give me permission - OR Push for me
On 2016/09/26 20:01:17, juliandoucette wrote: > I don't have permission to push to this repository. > > Can someone please: > - Give me permission > - OR Push for me Generally when you want to land a change for a repository which you don't have access to you should email a patch to the module owner or a peer. (A patch produced with Mercurial / Git so that it will include a commit message etc. Also important as diffs provided by Rietveld can't be trusted to even apply!) I guess in this case Vasily would be a good choice, he's module owner and also reviewed this change.
> I guess in this case Vasily would be a good choice, he's module owner and also > reviewed this change. Thanks, Dave. Indeed I'd be happy push this change. Julian, you can just do an `hg export ...` or `git format-patch ...` and send the patch to me (this way it will include the correct author and other meta stuff). BTW, Thomas and Manvel seem to have not given their LGTM. Do you guys have some reservations about this change or did you just wait for all the other feedback to be incorporated (then I suppose now is the time to take a look). Cheers, Vasily
On 2016/09/27 08:42:58, Vasily Kuznetsov wrote: > BTW, Thomas and Manvel seem to have not given their LGTM. Do you guys have some > reservations about this change or did you just wait for all the other feedback > to be incorporated (then I suppose now is the time to take a look). Quite honestly, five approvals in a review is way more than enough.
On 2016/09/27 08:42:58, Vasily Kuznetsov wrote: > BTW, Thomas and Manvel seem to have not given their LGTM. Do you guys have some > reservations about this change or did you just wait for all the other feedback > to be incorporated (then I suppose now is the time to take a look). Since we're not required to use it, don't feel obliged to wait for my approval.
> Quite honestly, five approvals in a review is way more than enough. > Since we're not required to use it, don't feel obliged to wait for my approval. Ok, I get the idea. Julian, feel free to send me the patch.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
