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

Migrate provider api #341

Merged
merged 11 commits into from
Jan 6, 2019
Merged

Migrate provider api #341

merged 11 commits into from
Jan 6, 2019

Conversation

adferrand
Copy link
Collaborator

Each of provider modules expose a subclass of the Provider class declared in the lexicon.providers.base module. The Provider class requires to implement four methods for the four actions available (create, list, update and delete). Each of these methods accept an argument named type.

Pylint complains about this, with reason: type is a special keyword in Python, as it corresponds to the function type() that gives the type of a reference. It means that currently, standard type() function is hidden by the type parameter of theses Provider method and thus cannot be used.

From Pylint point of view. I could just hide the error, but I would like to solve this situation in a more elegant way, in order to allow to use normally type().

Obsiously, the refactor implies to rename this type parameter to something else. I choosed rtype.

But doing this refactoring on the public methods of Provider subclasses would break the API, if you use theses methods through named parameters (eg. provider.list_records(type='TXT',...). Even if Provider subclasses should not be used directly, it is the case, in particular in Certbot. It could be corrected on upstream, but anyway retro-compatibility would be broken in an unacceptable range in my opinion.

So this PR tries to solve also the retro-compatibility issue in a way that makes also the code stronger: instead of requiring to implement the public methods, it is now some private methods that are abstract. Public methods are defined only on the base class and call the private methods.

This approach avoid to rewrite accidentally the API on some providers (I saw it during this PR, and corrected it). And it allows to implement any retro-compatibility processing required to preserve the API: here, the public methods take advantage of **kwargs to detect if type named parameter have been passed, and redirect its value to rtype.

This way, old codes will still work, without the need to hide the standard type() function. API refers officially now rtype, and a deprecation warning will be thrown if type is used in the relevant methods.

@adferrand adferrand self-assigned this Jan 3, 2019
@adferrand adferrand requested a review from AnalogJ January 3, 2019 21:20
@CapsuleCD
Copy link
Contributor

Hi.

I'm an automated pull request bot named CapsuleCD. I handle testing, versioning and package releases for this project.

  • If you're the owner of this repo, you can click the button below to kick off the CapsuleCD build pipeline and create an automated release.'
  • If not, don't worry, someone will be by shortly to check on your pull request.

CapsuleCD


If you're interested in learning more about CapsuleCD, or adding it to your project, you can check it out here

@AnalogJ
Copy link
Owner

AnalogJ commented Jan 4, 2019

ooof.
My naiive thought when I read your PR title was: "oh, he's just renaming type arguments to rtype, that should be pretty simple". I didn't even think about the named argument passing in python.

This looks like a pretty big change. I'd like to take a closer look at it this weekend, just so I understand whats going on. Though, the tests are passing, so I'm not too worried.

@adferrand
Copy link
Collaborator Author

Of course. The only real new logic is in lexicon/providers/base.py. Everything else is just refactor of method names and parameter rtype replacing type.

@AnalogJ
Copy link
Owner

AnalogJ commented Jan 5, 2019

ah, I see.
Looks good, I just wanted to make sure I understood the changes that were being made.
The only thing missing is updated text in the CONTRIBUTING.md file, stating the required provider methods.

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

Successfully merging this pull request may close these issues.

3 participants