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

Strict validation in configuration file (v2 only) #4607

Merged
merged 12 commits into from Oct 4, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Sep 5, 2018

Close #4455

I only need to add some docs. And maybe refactor a little.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 6, 2018

Ok, I think this is done!

Loading

@stsewd stsewd requested a review from Sep 6, 2018
after the key, if no value is passed,
it raises an exception when the key is not found.
"""
raise_ex = not bool(args)
Copy link
Member Author

@stsewd stsewd Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... I don't like much this hack, but I didn't find a better way to handle this. We can't put something like default=None, because we may need to default some values to None sometimes too. And I didn't want to add another parameter like default=None, raise_ex.

Loading

Copy link
Contributor

@agjohnson agjohnson Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't the method just be:

def pop_config(self, key, default=None, raise_ex=True):
    return self.pop(key.split('.'), self.raw_config, default, raise_ex

This still works if normal usage is:

self.pop_config('formats', [])

Loading

Copy link
Member Author

@stsewd stsewd Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to have another param here, but looks better than my hacky solution p:

Loading

Copy link
Member Author

@stsewd stsewd Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise_ex should be False here by default, so when we call self.pop_config('key', 'default') we don't need to pass another value like self.pop_config('key', 'default', raise_ex=False). But when we don't provide a default value, we need to called it like self.pop_config('key', raise_ex=False).

I'm trying to replicate the same behaviour from https://docs.python.org/3/library/stdtypes.html#dict.pop. Looks like the implementation is inside the c code.

Loading

Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like raise_ex=False would be the default value then. Does than not solve your use case?

Loading

Copy link
Member Author

@stsewd stsewd Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already implemented that in the last commit!

Loading

Copy link
Contributor

@agjohnson agjohnson Sep 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise_ex = not bool(args) and the rest of this method still seem like a non-standard way of doing a standard thing to me:

def pop_config(self, key, default=None, raise_ex=False):
    if default is None:
        raise_ex = True
    return self.pop(key.split('.'), self.raw_config, default, raise_ex)

This seems to be the same logic without having to manually handle *args and hacking raise_ex

Loading

Copy link
Member Author

@stsewd stsewd Sep 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we have more cases where default=None and shouldn't raise an exception, we only have one use of default=None and raise_ex=True. I don't think is a good idea to modify args based on other args.

This is the current implementation

https://github.com/stsewd/readthedocs.org/blob/c17e81c975a0639d56fc6028e90f80c5598bd3e7/readthedocs/config/config.py#L196-L204

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

👍 This is a nice way of implementing a check for left over keys. Noted some minor cleanup

Loading

readthedocs/config/config.py Show resolved Hide resolved
Loading
after the key, if no value is passed,
it raises an exception when the key is not found.
"""
raise_ex = not bool(args)
Copy link
Contributor

@agjohnson agjohnson Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't the method just be:

def pop_config(self, key, default=None, raise_ex=True):
    return self.pop(key.split('.'), self.raw_config, default, raise_ex

This still works if normal usage is:

self.pop_config('formats', [])

Loading

if wrong_key:
self.error(
wrong_key,
'Unsuported configuration: {}. Maybe a typo?'.format(wrong_key),
Copy link
Contributor

@agjohnson agjohnson Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo at Unsuported.

Also, perhaps Unsupported configuration option: {} for more specificity.

Maybe a typo? might not be clear for non-english speakers as well, so maybe something more formal here?

Loading

Copy link
Member Author

@stsewd stsewd Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the most formal sentence that I came up 😞

Loading

Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you honestly probably don't need the second portion of the error message. I think "unsupported configuration option: {x}" says enough

Loading

Copy link
Member Author

@stsewd stsewd Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already changed in the last commit

Loading

@agjohnson agjohnson added this to the YAML File Completion milestone Sep 19, 2018
Copy link
Member

@humitos humitos left a comment

I think this PR works, but I'd like to see some more tests and improve the docstring because I think the flow is confusing when you read the code for the first time.

I'm marking as "Comment" my review though, to not be that strict for these changes.

Loading

'build.extra'
)
])
def test_strict_validation(self, value, key):
Copy link
Member

@humitos humitos Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may worth to have some unit test for the specific methods that do the trick: _get_extra_key and validate_keys.

Loading

Copy link
Member

@humitos humitos Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular the one that is recursive with different start/stop conditions.

Loading

Copy link
Member Author

@stsewd stsewd Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate_keys is already called when running validate. _get_extra_key is validated here https://github.com/stsewd/readthedocs.org/blob/strict-validation/readthedocs/config/tests/test_config.py#L1718-L1743

Loading

Copy link
Member Author

@stsewd stsewd Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added one more test, but we are already covered anyway with the other tests (they don't throw an exception).

Loading

Copy link
Member

@humitos humitos Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not throwing an exception doesn't test that _get_extra_key returns the correct keys. That's the test I wanted.

The same for validate_keys to check that if there is extra keys, it fails.

Loading

Copy link
Member Author

@stsewd stsewd Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually test_strict_validation test that, an error is thrown and key and code are the correct in the error. We test with validate as entrypoint, as that's the way to use the config module (all tests were changed to test like that)

Loading

Copy link
Member

@humitos humitos Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you say. Although, I'd like to have a unit test for the methods I mentioned before for these cases:

  • _get_extra_key
    • returns no extra keys
    • a couple of cases where it returns extra keys
  • validate_keys
    • a couple of cases where it raises
    • the same for no raising the exception

The test_strict_validation is testing only one case and using the complete flow. I'd like to test the inner pieces, in particular the than that it's recursive (it's very easy to make a mistake on initial conditions)

Loading

"""
Checks that we don't have extra keys.
This should be called after all the validations are done.
Copy link
Member

@humitos humitos Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Can you expand a little more here?

If we know that we have an extra key and the build will fail because of this, why we are going to validate all the values first if we already know that it will fail?

Loading

Copy link
Member

@humitos humitos Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got it now, but it's hard to read.

I suppose that when validating the keys we pop (remove) these keys from the raw_config and finally if there are some leftovers it means that we have invalid keys.

If this is the way it works, it would be good to clarify in the docstring.

Loading

Copy link
Member Author

@stsewd stsewd Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that is

Loading

submodules['recursive'] = validate_bool(recursive)

return submodules

def validate_keys(self):
"""
Checks that we don't have extra keys.
Copy link
Member

@humitos humitos Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say Checks that we don't have invalid keys.

Loading

This should be called after all the validations are done.
"""
msg = (
'Unsupported configuration option: {}. '
Copy link
Member

@humitos humitos Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say Invalid configuration option:

Loading

Copy link
Member Author

@stsewd stsewd Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes more sense here

Loading

'Unsupported configuration option: {}. '
'Make sure the key name is correct.'
)
self.pop_config('version', None)
Copy link
Member

@humitos humitos Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Loading

Copy link
Member Author

@stsewd stsewd Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always pass the version key, but that's is already validated from outside

Loading

@agjohnson agjohnson removed this from the YAML File Completion milestone Oct 2, 2018
@agjohnson agjohnson added this to the 2.7 milestone Oct 2, 2018
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Oct 2, 2018

Changes look good to me, when @humitos gives the 👍 here, feel free to rebase/fix conflicts and merge!

Loading

@stsewd stsewd force-pushed the strict-validation branch from aef1b9f to d33854a Oct 3, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 3, 2018

Loading

"""
Get the extra keyname (list form) of a dict object.
If there are more the one key, the first one is returned.
Copy link
Member

@humitos humitos Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"more than one extra key"

Loading

humitos
humitos approved these changes Oct 4, 2018
Copy link
Member

@humitos humitos left a comment

Excellent! Thanks for the tests 😍

I left a comment for a typo :)

Loading

@stsewd stsewd merged commit 35695d1 into readthedocs:master Oct 4, 2018
1 check passed
Loading
@stsewd stsewd deleted the strict-validation branch Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants