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

[API change] Deprecating create in observers, v2: by fusing methods #578

Conversation

gabrieldemarmiesse
Copy link
Collaborator

@gabrieldemarmiesse gabrieldemarmiesse commented Aug 7, 2019

Follow up on #541 based on the recommendation of @Qwlouse

@gabrieldemarmiesse gabrieldemarmiesse changed the title Removing create try 2 Deprecating create in observers Aug 7, 2019
@gabrieldemarmiesse gabrieldemarmiesse marked this pull request as ready for review August 7, 2019 14:27
@gabrieldemarmiesse gabrieldemarmiesse changed the title Deprecating create in observers [API change] Deprecating create in observers Aug 7, 2019
@coveralls
Copy link

coveralls commented Aug 7, 2019

Coverage Status

Coverage increased (+0.06%) to 85.366% when pulling 7a228c2 on gabrieldemarmiesse:removing_create_try_2 into 2b1e756 on IDSIA:master.

@JarnoRFB
Copy link
Collaborator

This is more complicated then expected :D However, I think that the current proposal is not really a good idea as it exposes an argument mock_args to users leaking an api only needed for testing. What @Qwlouse proposed in #541 (comment) was a bit different. However, thinking about it I am not sure if it good to put the burden of finding the right parameter combinations on the users. Even though the original change was abusing inheritance, it felt much cleaner from an API perspective.

@gabrieldemarmiesse
Copy link
Collaborator Author

gabrieldemarmiesse commented Aug 13, 2019

@JarnoRFB I don't really understand this part:

However, I think that the current proposal is not really a good idea as it exposes an argument mock_args to users leaking an api only needed for testing. What @Qwlouse proposed in #541 (comment) was a bit different.

#541 (comment) Exposes engine and session in the user facing API, leaking an API only needed for testing too, right?

Even though the original change was abusing inheritance, it felt much cleaner from an API perspective.

I believe so too.

@JarnoRFB
Copy link
Collaborator

JarnoRFB commented Aug 13, 2019

#541 (comment) Exposes engine and session in the user facing API, leaking an API only needed for testing too, right?

Fair enough :D It probably feels like this, because it is called mock_args, which makes it more explicit.

@gabrieldemarmiesse gabrieldemarmiesse changed the title [API change] Deprecating create in observers [API change] Deprecating create in observers by fusing methods Aug 13, 2019
@gabrieldemarmiesse gabrieldemarmiesse changed the title [API change] Deprecating create in observers by fusing methods [API change] Deprecating create in observers, V2: by fusing methods Aug 13, 2019
@gabrieldemarmiesse gabrieldemarmiesse changed the title [API change] Deprecating create in observers, V2: by fusing methods [API change] Deprecating create in observers, v2: by fusing methods Aug 13, 2019
@gabrieldemarmiesse
Copy link
Collaborator Author

Alternative proposition: #541

@Qwlouse
Copy link
Collaborator

Qwlouse commented Aug 14, 2019

I'd prefer engine and session over mock_args, because they are more descriptive and they might be useful to some users (not only for testing, but also for handling obscure custom DB configurations).
I would just document them as a way to bypass the URL and manually set up the database connection.
It would also keep the interface more compatible to before, though that is not much of a concern.

If you feel that it is a problem to have this option openly exposed, I'd suggest catching the arguments with a generic **kwargs instead. That way we can hide them from the user almost completely.

@gabrieldemarmiesse
Copy link
Collaborator Author

Closing this in favor of #601

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

4 participants