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

feat: log every request to DSP-API #76

Merged
merged 19 commits into from
Mar 26, 2024
Merged

feat: log every request to DSP-API #76

merged 19 commits into from
Mar 26, 2024

Conversation

jnussbaum
Copy link
Collaborator

@jnussbaum jnussbaum commented Mar 22, 2024

Basically, this PR copy-pastes the Connection class of DSP-TOOLS, so that the permissions-related modules don't have to care any more how to make a request. As a plus, I don't pass around the connection object, but it lives in a single module, and all modules just access it there. This spares a lot of code!

@jnussbaum jnussbaum self-assigned this Mar 22, 2024
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

Over all, certainly a big improvements.

Here are some detail remarks, though:

  1. I don't like the connection.con.xyz(). if you called the instance connection too, you could import it from xyz.connection import connection, then you'd end up with calling connection.xyz() which IMO reads a lot nicer.
  2. I always hated the name "Connection" because that's not actually what it is. It knows how to handle a connection, but it is not a connection. It provides functionality to do calls to DSP, so what it actually is, is a client to talk to a DSP server, so a name along those lines (DspClient? and the instance could be called client?) would make more sense. I didn't want tu stir that pot in DSP-TOOLS, but I dislike to see it copied over here. What do you think?
  3. The singleton connection spares you some arguments to pass around, but it is conceptually the opposite of dependency injection, and introduces rock-solid coupling across the entire thing. I'm not sure how much testing needs to happen in these kinds of scripts; but if you do want to have good coverage, this will come to bite you eventually.
  4. if you decide to stick with the singleton connection, then it doesn't make much sense anymore to model it as a class which has a single instance into which you do (pseudo)-static calls. Then it could just as well be a module with top level functions and a number of private variables. The only reason to have classes is to be able to have multiple instances of them. And with this design, you also wouldn't need to do a first instantiation with Connection("foo") just to make typing happy [in all code paths, this instance gets overwritten with a different instance].

All these points are somewhat cosmetic, so feel free to ignore them or address them later.

dsp_permissions_scripts/utils/authentication.py Outdated Show resolved Hide resolved
@jnussbaum jnussbaum merged commit 933a5f7 into main Mar 26, 2024
1 check passed
@jnussbaum jnussbaum deleted the wip/enable-logging branch March 26, 2024 12:47
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