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

Support none docstrings #385

Merged

Conversation

jcharlong
Copy link

@jcharlong jcharlong commented Jun 13, 2024

Hello,

I ran into an issue running sdr code with CPython's -OO flag which optimizes out docstrings. The issue can be reproduced by running tests with python -OO -m pylint tests/.

This branch contains a solution broken into 2 commits:

  1. Check for and handle None docstrings within sdr's helper function.
  2. Require galois version 0.3.9 which brings support for CPython's -OO flag.

My branch is based off of release/0.0.x & I've ensured it passes ruff & the unit tests locally (w/ -OO).

Let me know if there's anything I missed or can do to facilitate merging this PR. Thank you in advance.

Justin Charlong added 2 commits June 13, 2024 14:59
CPython's -OO flag optimizes out docstrings and sets them to None.
New glois dependency is required to make use of CPython's -OO flag.
@mhostetter
Copy link
Owner

Thanks for the PR. Looks good to me!

One note, I noticed in your PR to galois that your commits aren't linked to your GitHub profile. (If you hover over your name in the commit history, it doesn't link to your GitHub page.) Because of that, you don't automatically appear in the "Contributors" list for this repo. If you care about that, I think this link could help.

image

@jcharlong
Copy link
Author

Ah, thanks. I hastily access to my personal GitHub account on my work machine for these 2 PRs. I wouldn't bother holding up the PR for this, but I appreciate the head's up & will address it.

@mhostetter mhostetter merged commit 507c802 into mhostetter:release/0.0.x Jun 13, 2024
26 checks passed
@mhostetter
Copy link
Owner

Thanks for this! I was planning on releasing the next version on Sunday. I can easily push a release tonight though, if you're looking to pull the latest sooner.

@jcharlong
Copy link
Author

Thanks for the quick merge & support. Sunday would work well, I don't think any sooner makes a difference on my end.

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