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

Manifest knows about providers #46

Merged
merged 4 commits into from
Apr 5, 2016
Merged

Manifest knows about providers #46

merged 4 commits into from
Apr 5, 2016

Conversation

sudssm
Copy link
Owner

@sudssm sudssm commented Apr 5, 2016

@DoronShapiro merge when you can

@@ -412,3 +420,13 @@ def generate_files_under(self, path):
yield child
else:
frontier.append(child)

def set_providers(self, providers):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these tested?

Copy link
Owner Author

Choose a reason for hiding this comment

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

no...

Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be too hard to test, let's do it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

@DoronShapiro
Copy link
Collaborator

What else is going on with providers here?

@sudssm
Copy link
Owner Author

sudssm commented Apr 5, 2016

Just some renaming and pretty printing in preparation for the next 2 prs that we discussed.

@@ -111,3 +110,28 @@ def expose_to_client(self):
Whether this provider should be exposed in user interfaces
"""
return False

@staticmethod
def type():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might call this something else, type sounds like it should be a python feature

Copy link
Owner Author

Choose a reason for hiding this comment

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

kind_of_provider seems verbose.
Since it's a class method, I think it's fine.
unless you have a suggestion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

name? provider_name?

@DoronShapiro
Copy link
Collaborator

Did a first pass

@sudssm
Copy link
Owner Author

sudssm commented Apr 5, 2016

@DoronShapiro done, take a look?

@DoronShapiro DoronShapiro merged commit 16cc343 into master Apr 5, 2016
@DoronShapiro DoronShapiro deleted the provider_manifest branch April 5, 2016 20:27
@DoronShapiro DoronShapiro restored the provider_manifest branch April 5, 2016 20:28
@DoronShapiro DoronShapiro deleted the provider_manifest branch April 5, 2016 20:28
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.

2 participants