New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-32305: Some speed ups in Config #595
Conversation
It's far too big to be defined inside a method so move it out as a standalone helper function.
This is a minor optiimization to stop the hierarchical key check looking for a sequence when it must be a dict.
This saves some time checking for cases that can't ever happen.
Codecov Report
@@ Coverage Diff @@
## master #595 +/- ##
=======================================
Coverage 83.51% 83.52%
=======================================
Files 241 241
Lines 30266 30275 +9
Branches 4520 4521 +1
=======================================
+ Hits 25277 25286 +9
Misses 3793 3793
Partials 1196 1196
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments about comments and one code question. Changes approved for merging.
nextVal = d[int(k)] | ||
isThere = True | ||
except IndexError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with all of the Config code. With that in mind, can d be a list of numbers (d = [10,20,30])? If yes (list is an instance of collections.abc.Sequence), and if k is 30, then it will get an IndexError and isThere will be False, but based on the ValueError line below I think we're wanting isThere to be True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It's something like config[".a.2.b"]
and a
refers to a sequence, 2
to index 2 and then b
is a key in the dict inside that sequence. It's features like this that are great for one off lookups in a config but a disaster when doing many of them because you get the isinstance check for sequence every time (except now I've bypassed it for a non-hierarchical look up since it's impossible for a Config top level to be a sequence.
Some minor optimizations but nothing that makes a huge difference.
Checklist
doc/changes