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

Refactor configuration object to class based #4298

Merged
merged 64 commits into from
Jul 10, 2018

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jun 25, 2018

Hopefully, this will solve #3806 (or maybe it will be split in two PRs).

I'm refactoring the existing code to be class based instead of using a dictionary, this will allow us to extend the behavior and have a common interface for multiple versions of the configuration file.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@stsewd stsewd added the PR: work in progress Pull request is not ready for full review label Jun 25, 2018
stsewd added 3 commits June 25, 2018 14:32

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
raise NotImplementedError()


class BuildConfig(BuildConfigBase, dict):
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm keeping the dict thing here for a while, so the refactor isn't too big

@stsewd
Copy link
Member Author

stsewd commented Jun 25, 2018

Please ignore the linter errors p:

@@ -442,11 +613,6 @@ def validate(self):
for build in self:
build.validate()

def set_output_base(self, directory):
Copy link
Member Author

Choose a reason for hiding this comment

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

We weren't using this, and it's the only setter. We should keep all the config object as read-only I think.

@stsewd
Copy link
Member Author

stsewd commented Jul 6, 2018

I just removed the ConfigWrapper class, tests are only failing in test_config_wrapper for obvious reasons p: Tomorrow I'll see what tests we can port to test_config.

@stsewd
Copy link
Member Author

stsewd commented Jul 6, 2018

I have tested this with some projects from github that have a readthedocs.yml file, it works as expected so far. I'll be re-reviewing all the code to see if there is something missing.

raise NotImplementedError()

@property
def python_interpreter(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can move this to a function outside the config object

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it makes somewhere else, yeah. We can probably leave for just now. Feel free to open an issue of you want to track this work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #4343

self.validate_conda()
self.validate_requirements_file()
self.validate_conf_file()
# TODO: this isn't used
Copy link
Member Author

Choose a reason for hiding this comment

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

I left some todos about things we aren't using right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

funny that we aren't attempting to use some of these yet, they could fix some buggy behavior. either way, we can add the functionality in v2.

@stsewd
Copy link
Member Author

stsewd commented Jul 6, 2018

I just left some comments about things we can remove (maybe in other PR), I think this is ready. Next step is creating the version2 and the class selector of the configuration object.

@stsewd stsewd removed the PR: work in progress Pull request is not ready for full review label Jul 6, 2018
@stsewd stsewd requested a review from agjohnson July 6, 2018 22:06
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Looks great, i think this is close to merge.

I noted what I think could be improved with the attribute methods from configwrapper (configbase in this PR). We can talk about this more. There isn't anything wrong with the implementation now, but it feels like a lot of unnecessary code to maintain, when we could just map values to a struct. Not something that we need to fix here.

super(ConfigNotSupportedError, self).__init__(
template.format(self.configuration),
CONFIG_NOT_SUPPORTED
)
Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like this is describing an unsupported configuration option instead perhaps? It's a little confusing referring to this as just unsupported configuration, as that implies to me a whole file or something.

So perhaps this class would instead be ConfigOptionNotSupportError, and the copy/docs can be updated

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

raise NotImplementedError()

@property
def python_interpreter(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it makes somewhere else, yeah. We can probably leave for just now. Feel free to open an issue of you want to track this work.

self.validate_conda()
self.validate_requirements_file()
self.validate_conf_file()
# TODO: this isn't used
Copy link
Contributor

Choose a reason for hiding this comment

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

funny that we aren't attempting to use some of these yet, they could fix some buggy behavior. either way, we can add the functionality in v2.

@@ -256,7 +317,7 @@ def validate_build(self):
"""
# Defaults
if 'build' in self.env_config:
build = self.env_config['build']
build = self.env_config['build'].copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@property
def build_image(self):
"""The docker image used by the builders."""
return self._config['build']['image']
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple things about the config attribute and usage pattern, which we can probably push off an resolve with a future PR:

  • There is a lot of repetition here, for code that we're just using to convert a dict -> object with attributes
  • We have multiple patterns for option naming: python_version vs install_project
  • This pattern flattens some of the options, from ['python']['version'] to python_version, loss of nesting makes some of the options less obvious.

A couple of options here:

  • Treat the config as a dictionary
  • Rename some of the attributes now, so that they retain more contextual information -- ie, rename some of the nested options to python_*
  • Use a namedtuple or a package like bunch to create nested attribute accessor instances, instead of repeating the logic above for each accessor

I think we can make a new issue here and address this later though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was thinking the same, maybe a namedtuple is enough, right now I just copy the names from the ConfigWrapper object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #4346

'output_base': '',
'type': 'sphinx',
'name': version.slug,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything you should do here before this is merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this would be fixed if we delete this options, they aren't used anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, having hard coded this options here isn't a problem right now, but isn't correct either p:

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

Successfully merging this pull request may close these issues.

None yet

2 participants