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

In cases where property names cannot be parsed, log a warning instead of raising a KeyError #321

Conversation

programmarchy
Copy link
Contributor

  • Issue #: KeyError caused by name stream not parsing correctly #320
  • Have you listed any changes to install or build dependencies? No changes.
  • Ensured your changes are compatible with Python 2.7 (ONLY FOR v0.29).
  • Have you updated the CHANGELOG with details of changes to the codebase, this includes new functionality, deprecated features, or any other material changes.
  • If necessary, have you bumped the version number? We will usually do this for you.
  • Have you included py.test tests with your pull request. (Not yet necessary)
  • Ensured your code is as close to PEP 8 compliant as possible?
  • Ensured your pull request is to the next-release branch (or v0.29 if applicable)?

If you haven't completed the above items, please wait to create a PR until you have done so. We will try to review and reply to PRs as quickly as possible.

Once your PR is approved by a maintainer, we will either merge it into next-release or do a release with you or for you.

Thanks for contributing!

streamID = properHex(0x8000 + entry['pid'])
self.__properties.append(StringNamedProperty(entry, names[entry['id']]) if entry['pkind'] == NamedPropertyType.STRING_NAMED else NumericalNamedProperty(entry))
if entry['id'] in names:
streamID = properHex(0x8000 + entry['pid'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not directly related to the issue, but looks like this line could be removed since streamID is unused.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gonna take me a bit to look over the changes and what might have went wrong in the code to see what the best option is. Meanwhile, can you tell me how you did this thing that highlighted a section of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheElementalOfDestruction Sounds good. I used this hex editor: https://hexfiend.com

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be clear, I meant the highlighted section of code in the pull request I am responding to. Some kind of code suggestion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also this suggestion is 100% the correct course. Pretty sure that was just something that got missed when I changed that section of code, so that line can just be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right. I just added a comment by clicking the "+" on that line in the Files changed tab.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, thank you. Learned something new. Also sorry if you got a bunch of notifications, I was having a little trouble with using github

@TheElementalOfDestruction
Copy link
Collaborator

As for the if statement, it looks more like there is an error of some kind in the parsing which is critical and should not be suppressed. In the event it isn't my code (I'm checking my code to see if I can figure out what is happening), then I'll use this strategy

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