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
base: master
from

Conversation

Projects
None yet
3 participants
@stsewd
Member

stsewd commented Feb 25, 2018

This fix rtfd/readthedocs.org#2813

  • Test case
  • Solution
@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Feb 25, 2018

Member

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

Member

stsewd commented Feb 25, 2018

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

@humitos

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

This comment has been minimized.

@humitos

humitos Feb 26, 2018

Member

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 ''
@humitos

humitos Feb 26, 2018

Member

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 ''
@agjohnson

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

Show outdated Hide outdated readthedocs_build/config/test_config.py
@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Apr 18, 2018

Member

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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Apr 18, 2018

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd May 10, 2018

Member

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

Member

stsewd commented May 10, 2018

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

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Jun 19, 2018

Contributor

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

Contributor

agjohnson commented Jun 19, 2018

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

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd
Member

stsewd commented Jun 19, 2018

@agjohnson yeah!

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Jun 19, 2018

Contributor

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

Contributor

agjohnson commented Jun 19, 2018

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

stsewd added a commit to stsewd/readthedocs.org that referenced this pull request Aug 2, 2018

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 2, 2018

Member

This is ported in rtfd/readthedocs.org#4461

Member

stsewd commented Aug 2, 2018

This is ported in rtfd/readthedocs.org#4461

@stsewd stsewd closed this Aug 2, 2018

@stsewd stsewd deleted the stsewd:allow-explicit-default-null branch Aug 2, 2018

agjohnson added a commit to rtfd/readthedocs.org that referenced this pull request Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment