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

22: Making a sub-package for server functions #25

Merged
merged 5 commits into from
Mar 27, 2024
Merged

Conversation

daffidwilde
Copy link
Contributor

Fixes #22

@daffidwilde daffidwilde requested a review from matweldon March 27, 2024 13:03
Encrypted data encryption key (used to encrypt the data).
"""

bucket = store.get_bucket(f"{party}-bucket")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this hard-coded? Does it conflict with the advice in the tutorial about creating hashed bucket names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because the users don't create the bucket themselves. You can see this naming structure in the party set-up script.


credentials = create_impersonation_credentials(party, operator)
store = storage.Client(credentials=credentials)
bucket = store.get_bucket(f"{party}-bucket")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about hard-coding

@@ -0,0 +1,46 @@
"""Functions for performing matching locally."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

To my taste, the functions in 'local.py' are too short and not used enough times to be worth wrapping up into a separate module. There's a readability trade-off: on the one hand it's tidy, but on the other hand it becomes much less linear; it requires the reader to search around to follow the underlying logic. I'd negotiate this trade-off differently, but you have the final say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see that for sure.

My view is that because the cloud functions get taken out (and they should) these should as well. Creating a module for each behaviour creates consistency, which is no bad thing.

Also, if we don't take them out, then the server script has stuff in it that isn't relevant for most users should they go delving into it.

@matweldon matweldon merged commit 926b625 into main Mar 27, 2024
4 checks passed
@matweldon matweldon deleted the 22-server-functions branch March 27, 2024 15:07
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.

Consolidate server functions into sub-package (like pprl.app)
2 participants