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

Add enum for DatastoreType used in Datastore objects, removed isSQL, … #179

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

nozmore
Copy link
Collaborator

@nozmore nozmore commented Sep 30, 2021

…updated sample tm, and updated existing tests

…updated sample tm, and updated existing tests
@nozmore nozmore requested a review from izar as a code owner September 30, 2021 19:50
@nozmore
Copy link
Collaborator Author

nozmore commented Sep 30, 2021

Let me know if anyone has thoughts on values for DatastoreType enum.

@@ -214,7 +216,7 @@ def test_defaults(self):
{"target": func, "condition": "not any(target.inputs)"},
{
"target": server,
"condition": "any(f.sink.oneOf(Datastore) and f.sink.isSQL "
"condition": "any(f.sink.oneOf(Datastore) and f.sink.type == DatastoreType.SQL "
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we'd use subclasses instead of a dedicated attribute? It looks weird to do two different kind of type assertion here, oneOf() and type == Enum, instead we could stick to only oneOf(). It also makes it easier to extend pytm by defining subclasses, as adding values to an existing Enum would require monkey-patching.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless we think there would be specific annotations for the subclasses I'd rather leave them as Datastore with annotations for what it is .. but I guess I could subclass DatastoreType then you could oneOf the type and accomplish the same thing.

@nozmore
Copy link
Collaborator Author

nozmore commented Sep 30, 2021

I realized the readme and docs have to get updated as well. I haven't looked into how the doc html is generated yet but will before merge.

@nineinchnick
Copy link
Collaborator

I haven't looked into how the doc html is generated yet but will before merge.

Just run make docs

@izar izar merged commit 91b6bbc into master Oct 29, 2021
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.

4 participants