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

Fix platform filter value for querying Treeherder jobs (#388) #389

Closed
wants to merge 0 commits into from

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Feb 12, 2016

This PR will fix issue #388 and also adds a couple more tests to check that the call chain for Treeherder API endpoints works correctly.

except:
value = values[0]

if api_endpoint == 'jobs':
Copy link

Choose a reason for hiding this comment

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

Seems like if api_endpoint is not in ('jobs', 'job-log-url') then you can skip this options,values loop entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jog-log-url has a different data set as jobs. For jobs the list to filter is under results while for the other it's at the root level.

Copy link

Choose a reason for hiding this comment

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

I see that but my original comment is about something else: is if the endpoint is neither job-log-url nor jobs then you don't need to loop through options,values at all.

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, now I see. I will check this after dinner.

@mjzffr
Copy link

mjzffr commented Feb 12, 2016

@whimboo A few things look suspicious to me. Perhaps you can explain them away... :)

@whimboo
Copy link
Contributor Author

whimboo commented Feb 12, 2016

@mjzffr I pushed an update. Please let me know what you think about the loops, and if that is ok for now.

@whimboo
Copy link
Contributor Author

whimboo commented Feb 12, 2016

@mjzffr the latest commit should make you satisfied. :)

@mjzffr
Copy link

mjzffr commented Feb 12, 2016

@whimboo r+wc, which I leave completely to your discretion.

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

2 participants