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

Allow explicit default with null value #38

Closed
wants to merge 10 commits into from

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Feb 25, 2018

This fix readthedocs/readthedocs.org#2813

  • Test case
  • Solution
@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 25, 2018

For now I'm only checking for the requirements_file default key, should I check for all defaults?

Copy link
Member

@humitos humitos left a comment

Good job!

I left just a comment that a test that can be added to cover the a missing case and it will be ready to merge from my point of view.

config = parse(buf)
assert config[0]['base'] is None


Copy link
Member

@humitos humitos Feb 26, 2018

I think a good addition here would be this test also:

def test_parse_empty_value():
    buf = StringIO(u'base: ""')
    config = parse(buf)
    assert config[0]['base'] is ''

Copy link
Contributor

@agjohnson agjohnson left a comment

Looks great! I just noted a piece that could stick around for removal in a separate PR.

@@ -15,7 +15,6 @@
from .config import PYTHON_INVALID
from .validation import INVALID_BOOL
from .validation import INVALID_CHOICE
from .validation import INVALID_DIRECTORY
Copy link
Contributor

@agjohnson agjohnson Mar 6, 2018

Let's keep this line for now, in case we have some side effects from removing a line like this.

Copy link
Member Author

@stsewd stsewd Mar 6, 2018

Done!

@stsewd stsewd force-pushed the allow-explicit-default-null branch from e89c7e1 to b8c68be Mar 6, 2018
@humitos
Copy link
Member

@humitos humitos commented Apr 18, 2018

I think this PR is ready to be merged.

For now I'm only checking for the requirements_file default key, should I check for all defaults?

I suppose we should look for a general solution for this instead of repeat something like this for every field:

        if 'field' not in self.raw_config:
            return None
 
        field = self.raw_config['field']
        if field is None:
            return None

What do you think?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 18, 2018

@humitos I was thinking of that, but if you look closely, always there are some additional checks before the second step, and at the end, we only will save 1 line of code per method.

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 10, 2018

I forgot to delete one line on the merge 🤦‍♂️

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jun 19, 2018

@stsewd shall we do a PR into rtd core for this as well?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 19, 2018

@agjohnson yeah!

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jun 19, 2018

Cool. Setting to blocking for now, we'll revisit this after.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 2, 2018

@stsewd stsewd closed this Aug 2, 2018
@stsewd stsewd deleted the allow-explicit-default-null branch Aug 2, 2018
agjohnson added a commit to readthedocs/readthedocs.org that referenced this issue Aug 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants