Skip to content

Conversation

ponty33
Copy link
Member

@ponty33 ponty33 commented Jun 23, 2021

Changes

  • Added Multipass class to generate multipass token or URL
  • Updated doc
    Screenshot_20220427_162750

@ponty33 ponty33 self-assigned this Jun 23, 2021
Copy link
Collaborator

@hillairet hillairet left a comment

Choose a reason for hiding this comment

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

@ponty33 Can this be completed? What's left?

ponty33 and others added 2 commits December 7, 2021 15:23
Co-authored-by: Anthony Hillairet <ant@satel.ca>
@ponty33 ponty33 marked this pull request as ready for review December 7, 2021 23:31
@hillairet
Copy link
Collaborator

Copy your README.md to docs/index.md to fix the test.
These two files should be identical.

@hillairet hillairet removed the request for review from Nathan-Nesbitt April 27, 2022 21:54
@hillairet
Copy link
Collaborator

@lishanl This could be useful. Do you mind looking at it to to see if it fits the rest of the code.
@ponty33 @lishanl Obviously up to you guys to decide who has time/interest in resolving conflicts and update things.

I think multipass needs its own section in the docs so it appears clearly in the menu on the left.

@ponty33
Copy link
Member Author

ponty33 commented Apr 27, 2022

I think I can work on it, good start for me to be involved in Spylib
But wow, it has been 10 months

@ponty33 ponty33 requested a review from lishanl as a code owner April 27, 2022 22:39
@ponty33 ponty33 changed the title Adding multipass function ✨ Adding multipass function Apr 27, 2022
@ponty33
Copy link
Member Author

ponty33 commented Apr 27, 2022

@lishanl Is there a way to preview the doc?

@lishanl
Copy link
Collaborator

lishanl commented Apr 27, 2022

@lishanl Is there a way to preview the doc?

start your mkdocs locally to check https://satelcreative.github.io/spylib/development-contributing/#build-and-run-documentation-lazydocsmkdocs @ponty33

@ponty33
Copy link
Member Author

ponty33 commented Apr 27, 2022

Updated doc:
Screenshot_20220427_162750

@ponty33 ponty33 requested a review from hillairet April 27, 2022 23:29
Copy link
Collaborator

@lishanl lishanl left a comment

Choose a reason for hiding this comment

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

Thank you Frank for adding and completing this feature 😎

Copy link
Collaborator

@hillairet hillairet left a comment

Choose a reason for hiding this comment

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

@lishanl @ponty33 Let's talk about the design.

Is there a reason to have a class?

I would prefer a set of functions instead:

def generate_token(secret: str, contact_raw_data: Dict[str, Any]) -> bytes:
   ...

def generate_url(secret: str, contact_raw_data: Dict[str, Any], url: str) -> str:
    ...

The docs could show how to use partial to avoid passing around the secret.
The user can still have multipass.:

from spylib import multipass

url = multipass.generate_url(...

The small helper functions should start with _ (that applies to internal methods too):

def _get_keys(secret: str) -> Tuple[str, str]:
    key = SHA256.new(secret.encode('utf-8')).digest()
   return (key[0:16], key[16:32])

Bonus point if you use a dataclass or a pydantic model instead of a Tuple! 😉

@lishanl
Copy link
Collaborator

lishanl commented Apr 28, 2022

@lishanl @ponty33 Let's talk about the design.

Is there a reason to have a class?

I would prefer a set of functions instead:

def generate_token(secret: str, contact_raw_data: Dict[str, Any]) -> bytes:
   ...

def generate_url(secret: str, contact_raw_data: Dict[str, Any], url: str) -> str:
    ...

The docs could show how to use partial to avoid passing around the secret. The user can still have multipass.:

from spylib import multipass

url = multipass.generate_url(...

The small helper functions should start with _ (that applies to internal methods too):

def _get_keys(secret: str) -> Tuple[str, str]:
    key = SHA256.new(secret.encode('utf-8')).digest()
   return (key[0:16], key[16:32])

Bonus point if you use a dataclass or a pydantic model instead of a Tuple! 😉

OOP VS Functional programming 🥁

@hillairet
Copy link
Collaborator

@lishanl Well maybe ... is "multipass" really an object though?
I don't think having a bunch of class means doing OOP, if none of your classes actually represent objects.

@ponty33
Copy link
Member Author

ponty33 commented Apr 28, 2022

There wasn't much reason behind it. The code was referenced from an unmaintained repo, and it was written in class

I don't have much opinion on it, either way is fine to me

Copy link
Collaborator

@hillairet hillairet left a comment

Choose a reason for hiding this comment

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

@ponty33 Let's go with functions here please. No reason for a class.

@hillairet hillairet added this to the 4 - Enemy of the State milestone May 12, 2022
@ponty33 ponty33 requested review from hillairet and lishanl May 12, 2022 21:01
Copy link
Collaborator

@hillairet hillairet left a comment

Choose a reason for hiding this comment

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

I believe the email is a required field and the only required field in the customer data dict. Please add a check so that SPyLib fails with a meaningful error message before the user makes the multipass API call.

@ponty33 ponty33 requested a review from hillairet May 13, 2022 18:40
Copy link
Collaborator

@lishanl lishanl left a comment

Choose a reason for hiding this comment

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

I am picking on the consistency here. 😅 Hope you don't mind Frank. It would be nice to have type annotations for all functions.

Copy link
Collaborator

@hillairet hillairet left a comment

Choose a reason for hiding this comment

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

Remove unused thing. Otherwise looks great.

@ponty33 ponty33 requested a review from hillairet May 18, 2022 16:51
@ponty33 ponty33 merged commit 9cbafb9 into main May 18, 2022
@ponty33 ponty33 deleted the feature/multipass branch May 18, 2022 22:18
@ponty33
Copy link
Member Author

ponty33 commented May 18, 2022

Finally closed after 329 days 😄

@lishanl
Copy link
Collaborator

lishanl commented May 18, 2022

Definitely top 1 on the leaderboard for the longest open PR that actually gets completed at the end. 🏆
Not to encourage this. But yea. Glad it's completed. 😆

@hillairet
Copy link
Collaborator

I miss PullPanda for that! The listing of records and averages was great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants