Constants value order #4
Conversation
|
I don't know why there are 2 commits from the last PR, but it shouldn't impact. I will try to be more careful next time. |
lib/foobara/util/module.rb
Outdated
| extends = Util.array(extends) | ||
|
|
||
| mod.constants.map { |const| constant_value(mod, const) }.select do |object| | ||
| mod.constants.sort_by { |const| [const.to_s.downcase, const.to_s] } |
There was a problem hiding this comment.
I actually think we can get by with just .sort because the actual order isn't that important it's the need to eliminate non-determinism. There was a hard-to-debug test suite failure because Mac for whatever reason was giving the constants in a different order than Linux where the spec was written. So any order should work.
|
|
||
| it "returns the constants sorted by name" do | ||
| expect(described_class.constant_values(Foo)).to eq([Foo::AConst, Foo::Bar, Foo::BConst]) | ||
| end |
lib/foobara/util/constants.rb
Outdated
| @@ -0,0 +1,3 @@ | |||
| module Foobara | |||
| EMPTY_ARRAY = [].freeze | |||
| end | |||
There was a problem hiding this comment.
I think could rebase against foobara/util's main would get rid of these. Could also make a new branch and cherry pick your new changes. Or an interactive rebase in your branch to drop these commits.
|
This might work to get rid of the commits from the previous branch, assuming you don't have a remote setup to my repo: |
lib/foobara/util/module.rb
Outdated
|
|
||
| mod.constants.map { |const| constant_value(mod, const) }.select do |object| | ||
| mod.constants.sort | ||
| .map { |const| constant_value(mod, const) }.select do |object| |
There was a problem hiding this comment.
Ah, OK to leave this like this, but a tip I could give is I rarely break lines like this. The reason is if I copy/paste mod.constants.sort it will actually run if I copy/paste it into a REPL even though it's not the entire expression, which has bitten me before. But if we broke it somewhere else, like maybe changing { to do or whatever else, if I copy mod.constans.sort do |const| then the REPL will sit there waiting for more text because it's not a complete expression.
Another random thought and something I'm starting to do is one could shorten the line a bit with map { constant_value(mod, it) } though pretty subjective. Could also just introduce a local variable like constants = mod.constants.sort and then constants.map.... Lots of options.
There was a problem hiding this comment.
I will use 'do' instead of {.
And refactor the code to use constants = mod.constants.sort and then constants.map. It looks better for me too.
sherazp995
force-pushed
the
constants-value-order
branch
from
f408312 to
e6c08b1
Compare
|
Great!! I'll see if I can easily add cancelei as a co-author after merging |
Co-authored-by: Sheraz Aurang Zaib <Sherazp995@gmail.com>
|
Oh interesting the co-author stuff was already setup correctly but github wasn't showing it correctly in this PR for some reason |
3 participants
Proposal to close #2