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

Use flag instead of getattr as it was not working as expected. Provid… #14

Merged
merged 2 commits into from Apr 1, 2016

Conversation

alan-unravel
Copy link
Contributor

…ed raw pages instead of recursive call to har_parser.pages which reached the recursion limit on failure

…ed raw pages instead of recursive call to har_parser.pages which reached the recursion limit on failure
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 93.898% when pulling 99f4683 on alan-unravel:recursion_fix into 2f8d3d4 on mrname:master.

@alan-unravel
Copy link
Contributor Author

Not sure how the coverage decreased since I didn't touch anything except for some local variables and the way pages were accessed in an Error

@mrname
Copy link
Collaborator

mrname commented Apr 1, 2016

LOL, yes coveralls can be janky, don't worry about it 😄

'No page found with id {0}\n\nValid pages are {1}'.format(
self.page_id, self.parser.pages)
'No page found with id {0}\n\nPages are {1}'.format(
self.page_id, raw_data['pages'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would you think about just printing the page ID's here instead of the full pages dict? This output could probably get pretty confusing for a file with lots of HAR pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that could work, I went with the closest thing to accessing the property .pages. I think the id's would be sufficient since most errors would have to do with the id in this instance

@mrname
Copy link
Collaborator

mrname commented Apr 1, 2016

Aside from my comment about the error output everything looks pretty good @alan-unravel. Just curious, how did you run into this problem and what did the stack trace look like? Just wondering if we should add a regression test to address the issue you hit.

@alan-unravel
Copy link
Contributor Author

Just a simple run through the parser and requesting the HarPages, surprised the tests didn't catch this. The last pages getattr(self, 'title', None) was always resolving to None and then I was getting a maximum recursion depth exceeded.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 93.919% when pulling 1e1a56a on alan-unravel:recursion_fix into 2f8d3d4 on mrname:master.

@mrname
Copy link
Collaborator

mrname commented Apr 1, 2016

👍

@mrname mrname merged commit a9c8053 into haralyzer:master Apr 1, 2016
@alan-unravel alan-unravel deleted the recursion_fix branch April 1, 2016 14:53
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.

None yet

3 participants