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

Added pem data #74

Merged
merged 11 commits into from
Jun 20, 2023
Merged

Added pem data #74

merged 11 commits into from
Jun 20, 2023

Conversation

spacemanspiff2007
Copy link
Contributor

Pull Request Check List

This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do.

  • Added tests for changed code.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) are documented in the changelog.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

@spacemanspiff2007
Copy link
Contributor Author

Closes #73

Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

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

In addition to the review comments, how would you feel about adding a decoded_payload too? b64decode is part of the stdlib and it seems like most people who want to access the payload in the first place, probably also want to decode it.

src/pem/_core.py Outdated Show resolved Hide resolved
src/pem/_core.py Outdated Show resolved Hide resolved
src/pem/_core.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@spacemanspiff2007
Copy link
Contributor Author

spacemanspiff2007 commented Jun 20, 2023

how would you feel about adding a decoded_payload too?

Maybe use payload_raw and payload_bytes?
Or rather payload_decoded?

@hynek
Copy link
Owner

hynek commented Jun 20, 2023

I think payload_decoded is more precise/obvious, yes!

@spacemanspiff2007
Copy link
Contributor Author

Like this?

Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

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

yes, LGTM!

I think I'll make it more backwards-compatible and make payload optional with a warning, but we're done here – thanks!

@hynek hynek merged commit f4f39b7 into hynek:main Jun 20, 2023
14 checks passed
@spacemanspiff2007 spacemanspiff2007 deleted the pem-data branch June 20, 2023 07:28
@hynek
Copy link
Owner

hynek commented Jun 20, 2023

(I understand now why you put it into the breaking section :) – I kinda didn't consider the constructor to be a public API, but 101% someone somewhere is doing just that)

hynek added a commit that referenced this pull request Jun 20, 2023
@hynek
Copy link
Owner

hynek commented Jun 20, 2023

as a heads up: while making is compatible, I've decided the churn isn't worth it and switched to a lazy extraction in 0a9780a, I hope you can live with that.

@spacemanspiff2007
Copy link
Contributor Author

Yes - of course! Thanks for the heads up

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