Skip to content

Update docstrings formatting refactor#255

Open
bushjam1 wants to merge 3 commits into
pydicom:masterfrom
bushjam1:update-docstrings-formatting-refactor
Open

Update docstrings formatting refactor#255
bushjam1 wants to merge 3 commits into
pydicom:masterfrom
bushjam1:update-docstrings-formatting-refactor

Conversation

@bushjam1
Copy link
Copy Markdown

Description

This PR submits several minor formatting and docstring changes and one change to the logic of a function using multiple conditionals to use a dictionary instead. No changes are part of a current issue.

Related issues: # (issue)

None

Please include a summary of the change(s) and if relevant, any related issues above.

There are changes to several docstrings in the file (tags.py) to add parameters, formatting to reduce line lengths >100 chars, and one change to the function apply_filter() in filter.py to use dict map instead of conditionals.

Checklist

  • [X ] I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • My code follows the style guidelines of this project

Open questions

Questions that require more discussion or to be addressed in future development:

None. Thanks for reviewing!

Comment thread deid/dicom/filter.py
if "0x" in field:
field = int(field, 0) # 0=decode hex with 0x prefix
filter_name = filter_name.lower().strip()
filter_name_map = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see what you are trying to do, but this design isn't an improvement because in creating the dictionary you are executing each function. We definitely don't want to do that.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for the feedback.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you’d like to revert this change, the docstring fixes are great and would be much appreciated!

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