Skip to content
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

Style problems #6

Open
dcz-collabora opened this issue Jan 20, 2017 · 1 comment
Open

Style problems #6

dcz-collabora opened this issue Jan 20, 2017 · 1 comment

Comments

@dcz-collabora
Copy link
Contributor

While fixing some clarity problems in https://git.collabora.com/cgit/user/dcz/kernelci-backend.git/ , I found some additional things that are not so obvious and I may not have a chance to fix. This issue is to make sure they are not forgotten.

JSON data in app.utils.boot._parse_boot_from_json is not normalized. Values such as the strings "none", "null" are taken to be invalid for a few selected fields, but if any other field contains it, no special handling happens. Once the JSON object is deserialized, it should follow Python conventions (empty is None) to avoid mistakes like that. A solution would be to convert and discard original object immediately.

JSON validation is done manually. KCI has JSON schema written, so it could be a good idea to use the jsonschema package.

Copying dictionaries to object or dictionaries is a common pattern and makes functions very long and not too readable. It can be done in a short loop if either:

  • all elements from source are used
  • source and destination elements have matching names
@MiloCasagrande
Copy link
Contributor

Hi,

first, thanks for going through the cruft in those modules: they definitely need a refresh and clean up.

I'll start with this one first, since it's kind of related to other points.

JSON validation is done manually. KCI has JSON schema written, so it could be a good idea to use the jsonschema package.

Unfortunately, the jsonschema validator wasn't really an option at the beginning, due to what we had at the time. It could be now, but I wouldn't consider it with this code base (more on that later).

The JSON schema available now, or at least something that could resemble one (probably it's not even 100% valid), came after: we started with a data file that already had a structure and we worked on it, and it wasn't really clean. That's why we had to hack together things like "none" or "null" to accept the data.

We are working on a v2 version of the API (although at a slow pace) that will avoid those hacks/mistakes/assumptions made in the past and that will use the jsonschema validators. We are also trying to have just one schema, not split up into GET and POST, without mangling parameters name.

JSON data in app.utils.boot._parse_boot_from_json is not normalized. Values such as the strings "none", "null" are taken to be invalid for a few selected fields, but if any other field contains it, no special handling happens. Once the JSON object is deserialized, it should follow Python conventions (empty is None) to avoid mistakes like that. A solution would be to convert and discard original object immediately.

I think those can be clean up by now. The data provider are producing cleaner data: those hacks shouldn't be valid anymore and we could definitely start enforcing a better data structure.

Copying dictionaries to object or dictionaries is a common pattern and makes functions very long and not too readable. It can be done in a short loop if either:

all elements from source are used
source and destination elements have matching names

Matching names in elements is something we are working on with v2, and it should simplify, if not remove completely, all that code path. Same thing for using all elements without blindly throwing all the rest into the metadata field. The json schema will be enforced, and refuse anything that has undefined properties.

That said, cleaning up those code paths to make them more readable and robust is definitely welcomed, since they might still be used in the future and will definitely be used now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants