-
Notifications
You must be signed in to change notification settings - Fork 79
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: add repr
kwarg to field
function and Field
C struct
#543
Open
lmmx
wants to merge
11
commits into
jcrist:main
Choose a base branch
from
lmmx:customisable-field-repr
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Extended the field Python function to accept a new keyword argument `repr` which controls the inclusion of the field in the generated `__repr__` string for the struct. Added a new attribute `repr` to the `Field` C struct to store the value of the `repr` keyword argument. Updated the `Field` C struct docstring to include details about the new `repr` kwarg.
…ed format specifier, add to `Field_members` and `PyArg_ParseTupleAndKeywords`
… well; fix test nits
I've implemented support for callable arguments and added all the test coverage I think this feature needs now, looks ready to review @jcrist 🎉 Please let me know of any changes you'd like me to make |
This would be super valuable for e.g. not accidentally exposing secrets in logs. |
Would you like any changes to this to shepherd it along @jcrist ? 🙂 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I picked up ticket #492 to help contribute this feature, as I'd really like to use this library for a project which requires excluding fields (a linked list with mutual references which would produce infinite loop if
repr
'd).To begin, I have done the following:
repr
which controls the inclusion of the field in the generated__repr__
string for the struct.repr
to theField
C struct to store the value of therepr
keyword argument.Field
C struct docstring to include details about the newrepr
kwarg.Then to complete the feature I followed the lead of the other lookup dictionaries (ending in
_lk
) likedefaults_lk
, and used a list that was converted to a tuple, as I'm sure you're familiar. For boolean configuration I have only allowed this to bePy_True
orPy_False
, and defaulting toPy_True
if the value type is not aField
object. With a little extra logic here this could be extended to handle arbitary callables if desired.I tried to follow the example of how to represent
repr
in C from where other arguments useint
to represent booleans. Please take a careful look at this to ensure it fits the house style.Update I've added tests and remembered to also implement this feature in the
__rich_repr__
method internal too.Update I've implemented support for callables as described in the original ticket
Update I reviewed the memory management (incrementing/decrementing), refcount tests continue to pass