Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Fix a bug when using None values in a list of floats #2299
Conversation
|
I was able to run black and run the tests locally however initially some of the tests were failing. No other old tests started failing after my changes so hopefully everything is alright. I'm not sure if this warrants a mention in docs/changelog.rst Let me know if I put the tests in the wrong place. |
|
Seems like some of the travis ci tests failed. Looks like there is some issue with configparser. When installing locally I had to Edit: configparser released a new version 5.0.0 recently. Looks like you need to require |
|
I've fixed the problem with configparser, could you pull master in your branch? |
Adam-Edry
force-pushed the
Adam-Edry:add-float-field-none-value
branch
from
1c62a46
to
83c1446
|
Done. Thank you |
|
Just had a look at this. The PR looks great but I'm afraid that the actual "problem" is in the ListField, here you are fixing it for the FloatField but the problem remains for other fields (StringField, etc) so the behavior is not consistent across the fields. I'm trying to understand what could be the impact if we change that in the ListField itself |
|
I see. I haven't taken a look at ListField but I agree that inconsistent behavior is not what we want. Hopefully I will have time to take a closer look at ListField tomorrow. Although, given my unfamiliarity with this code base I may not be the best person to consider widespread impacts and whether or not that would be a breaking change. |
|
It seems to me that ListField defers the validation and to_python methods to whatever class the list element has. This happens in the ancestors of ListField - ComplexBaseField in to_python method and BaseField in _validate. I see two ways to approach this. I don't know which approach is better. I would lean towards the first because that is where there is currently an inconsistency. For example StringField's to_python function does work with None however validate does not. Same case with other fields. It was only a coincidence that I stumbled upon this issue with FloatField and its to_python method. I would be glad to hear your thoughts on this @bagerard |
Fixing a bug reported in issue #2290.
When trying to use None values in a List of Floats there would be an exception thrown from either to_python or validate methods.
Added two failing tests that cover each of those functions and then fixed them.