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

Update docstrings formatting refactor #255

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bushjam1
Copy link

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!

value: the value to set, if filter_name is valid

"""
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
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
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
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