finos / metadata-tool

A command line tool for performing various tasks with Fintech Open Source Foundation (FINOS) metadata.
Apache License 2.0
12 stars 5 forks source link

Applying style guide #33

Closed skuro closed 5 years ago

skuro commented 5 years ago
skuro commented 5 years ago

Thanks for the review!

There is admittedly 0 functional changes in this PR. While at the beginning I tried to configure my toolchain to respect the local styleguide (which I started to derive from existing code) I found inconsistencies in the project (e.g. 4 spaces vs 2 spaces indentation) and just let the tool do the job.

The main reason behind the PR it is that, being it a shenanigan or not, most clojure developers around the globe will have their emacs + clojure-mode reindent each file upon save and have to battle through this over and over. Read: I had some functional changes to add but I was finding myself either managing indentation by hand (manageable but not preferable) or just fighting the tool (which is quite frustrating).

I'll go through the comments one by one and provide some context.

pmonks commented 5 years ago

I am opposed to optimising code style for a specific editor (any editor), since it treats users of other editors as second class citizens. This kind of special treatment is the antithesis of open source’s inclusive approach to community building and collaboration.

As of 2019, less than half of Clojure developers use Emacs (myself included, fwiw) and so Emacs is no different in not deserving special treatment.

With that said, I have no objection to including editor-specific files in the codebase (the .dir-locals.el file, for example), provided that that doesn’t negatively impact the developer experience of users of other editors.

skuro commented 5 years ago

@pmonks 100% agreed, it is more to strive for inclusiveness than specific editor support that IMO having a unique style not shared by any other clojure project (to my knowledge) is a bad idea. Paredit / parinfer are not Emacs specific, only the .dir-locals.el is there for stadardization purposes plus me personally using Emacs.

pmonks commented 5 years ago

Great! Once you’ve reverted the PR to preserve the FINOS Clojure coding style, I’d be more than happy to merge the Emacs specific configuration file.

pmonks commented 5 years ago

I stand corrected - I no longer have commit access, so can't offer to merge any PRs. cc @maoo

skuro commented 5 years ago

I agree on the principles but not on the practice: the current style doesn't match that of any project I ever encountered in 10 years of clojure, which suggests to me that by itself is likely more of an entry barrier than anything else. The principles behind my PR are not rooted in Emacs but in common practice, and I fail to see how keeping the current style is helping non-Emacs users in any possible way.

The FINOS coding style is particularly unconventional, I personally find it visually noisy and I'm not interested into maintaining an emacs config for it: it takes me less to switch off any IDE support and manually keeping the current style. If that's the style guide that will be enforced I'd rather just remove the .dir-locals.el altogether. On a side note, is manual indentation common practice at FINOS or is there any tool involved?

pmonks commented 5 years ago

Given this is an internal FINOS tool with little to no utility outside the specific operations of the Foundation, appealing to external authorities isn’t especially relevant.

Furthermore, it’s quite an overstatement to imply that there is One True Code Style for Clojure code. For every example of the Emacs style you cite, I can produce examples of other Clojure code bases that don’t follow that style.

Until / unless core Clojure mandates a single style, and includes multi-editor tooling to support such a (draconian) measure, appealing to any given Clojure style is a canard.

skuro commented 5 years ago

For one, I'd welcome better cross-IDE clojure tooling to align on style: as a Lisp and therefore almost syntax-less, being able to trust your eyes is a fundamental tool under your clojure belt. That's the whole idea behind parinfer (which I don't use, by the way).

The clojure community isn't as libertine as you depict it when it comes to styling. I highly doubt you can find such variety in any relevant project that can confute things like "indent by 2 spaces" as a wildly accepted rule. And feel free to call mine Emacs style, but that's like me calling FINOS's the Style Against Developers Happiness™: politically aligned with my goals but not rooted into reality ;-)

"It works for us, that's the house style" (not your words but the general sentiment of your last comments) is something that I can understand and accept much better than "we don't care about Emacs" which is, IMO, straw man. I'm happy to close the discussion here and reopen it if and when my contributions will surpass yours.

p.s.: I'm genuinely interested to know whether you manage indentation / alignment manually or of you use any tool

pmonks commented 5 years ago

"It works for us, that's the house style" (not your words but the general sentiment of your last comments) is something that I can understand and accept

That's what I said originally (2nd para), and what is clearly spelled out in the contributing guide for the project as well (bullet points 2 & 6). I can't really help you if you don't RTFM. ;-)

skuro commented 5 years ago

Please realise that extant code style has various degrees of inconsistencies and non-cohesiveness (even in the same commit, e.g. here vs here, next to 22 files with 4 spaces indent vs 18 files with 2 spaces indent etc.). But you are right, I'll keep indentation changes to the bare minimum as per the contributing guide.

pmonks commented 5 years ago

"In the face of fixed scope, deadlines & budget, code style is one of the first victims..."

maoo commented 5 years ago

Thanks a lot @skuro for the PR and @pmonks for the review.

@skuro - as @pmonks said earlier, this project is mostly used internally to manage FINOS internal metadata, which makes me appreciate your contribution even more. If you're interested to continue helping along in the project, I'd be more than happy to change our contributing guide to make your work easier.

Thanks!