MINIFICPP-1288 - Refactor YamlConfiguration::parsePropertiesNodeYaml#901
MINIFICPP-1288 - Refactor YamlConfiguration::parsePropertiesNodeYaml#901hunyadi-dev wants to merge 6 commits intoapache:mainfrom
Conversation
fgerlits
left a comment
fgerlits
left a comment
There was a problem hiding this comment.
I like this; this function was insanely long and complex.
| try { | ||
| PropertyValue coercedValue = defaultValue; | ||
| if (defaultType == typeid(int64_t)) { | ||
| coercedValue = propertyValueNode.as<int64_t>(); |
There was a problem hiding this comment.
Hm... this has changed quite a bit. For example, why don't we need the case for uint64_t any longer?
Also, do we have enough unit tests to make sure the new code works in the same way as the old code?
There was a problem hiding this comment.
Honestly, I do not quite remember why we do not need uint64_t anymore :S
It probably has something to do with the uint64 nodes being parsed as string even though their value type matches here:
class DataSizeValue : public TransformableValue, public state::response::UInt64Value {There has been a discussion about it where @szaszm replied that he agreed on the removal, but gah this is old. This should be though thoroughly tested as there are many integration tests that directly rely on yamlparsing and even some yamlconfiguration tests as well.
There was a problem hiding this comment.
I don't remember the discussion, so some pointers to context would be appreciated. 🙂
hunyadi-dev
force-pushed
the
MINIFICPP-1288_parsePropertiesNodeYaml_cleanup
branch
from
8946cb2 to
1ce1fb0
Compare
…YamlConfiguration
…erty and improve const correctness
hunyadi-dev
force-pushed
the
MINIFICPP-1288_parsePropertiesNodeYaml_cleanup
branch
from
20645f8 to
39c38e3
Compare
3 participants
Splitting up YamlConfiguration::parsePropertiesNodeYaml and simplifying each of the parsing steps.