Finish formatting of PolyhedralGeometry #3626
Conversation
| const _johnson_indexes_from_oscar = Set{Int}([9, 10, 13, 16, 17, 18, 20, 21, 22, 23, 24, | ||
| 25, 30, 32, 33, 34, 35, 36, 38, 39, 40, | ||
| 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, | ||
| 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, | ||
| 61, 64, 68, 69, 70, 71, 72, 73, 74, 75, | ||
| 77, 78, 79, 82, 84, 85, 86, 87, 88, 89, | ||
| 90, 92]) | ||
| const _johnson_indexes_from_oscar = Set{Int}([ | ||
| 9, | ||
| 10, | ||
| 13, | ||
| 16, | ||
| 17, | ||
| 18, | ||
| 20, | ||
| 21, | ||
| 22, |
There was a problem hiding this comment.
Hmm, I'm not sure I like this change. If one scrolls down to the end of the list, one can't see the name _johnson_indexes_from_oscar anymore, at least on my screen. Apparently, one could leave it as it were and add comments for the formatter to ignore it (https://domluna.github.io/JuliaFormatter.jl/stable/#Turn-off/on-formatting) so that the tests pass, but I did not test this.
But you can also leave it like this; long lists of constants never look great I guess.
There was a problem hiding this comment.
I like the idea of putting some few noformat tags around things like this to keep files readable. That's why we even format them in the first place imo.
There was a problem hiding this comment.
I know that I can make the formatter ignore this. But maybe there is something to make it little more forgiving in general?
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3626 +/- ##
=======================================
Coverage 82.78% 82.78%
=======================================
Files 577 577
Lines 77743 77743
=======================================
+ Hits 64358 64359 +1
+ Misses 13385 13384 -1
|
lkastner
force-pushed
the
lk/format/polyhedral
branch
from
e80be3b
to
cae5980
Compare
| invA = [1 2 3 -4; | ||
| 1 0 1 -1; | ||
| 1 9 0 -6; | ||
| 1 1 5 -5] |
There was a problem hiding this comment.
I am not a fan of how this new option formats matrices like this. The offset of the first row is weird. I think without this option this was more aligned.
But let's hear other peoples opinions as well.
|
In general I would rather avoid anything were we manually interfere with the formatter since this always entails mentioning this in PRs and explaining it to devs. One main goal of the formatting tests is to reduce our work when reviewing PRs. It has the added benefit that there is an automated way for devs to fix their formatting. The many comments on formatting can seem a bit like micromanagement and be quite discouraging. |
|
What is the plan now? I would think that with every set of formatting rules, there will be some situation where the result is not optimal. And there will always be somebody who doesn't like something about the current style (I personally would put more white space than the formatter for example). |
No description provided.