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

add tests for [hackage] #1416

Merged
merged 4 commits into from
Jan 7, 2018
Merged

add tests for [hackage] #1416

merged 4 commits into from
Jan 7, 2018

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Jan 3, 2018

Note there is no check for the 'not found' case on the hackage-deps endpoint.

In order to add a not found case, we'd have to add a second API call so that the hackage-deps endpoint calls https://hackage.haskell.org/ first to check if the package exists and then makes a subsequent call to http://packdeps.haskellers.com if it does. Is it worth adding that additional overhead to cover the not found case?

@shields-ci
Copy link

shields-ci commented Jan 3, 2018

Messages
📖

✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS

@paulmelnikow
Copy link
Member

Hmm, returning up to date when the package name is misspelled seems pretty bad!

How about opening an issue at https://github.com/snoyberg/packdeps? If they could provide a way to distinguish them, and that could be resolved fairly quickly, that seems like a good way to go. Maybe they could even add a JSON API? 😀

FWIW the website is able to distinguish the two:

http://packdeps.haskellers.com/feed?needle=not-a-package
http://packdeps.haskellers.com/feed?needle=AbortT-mtlz

@paulmelnikow paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Jan 4, 2018
@chris48s
Copy link
Member Author

chris48s commented Jan 4, 2018

I've raised an issue for it: snoyberg/packdeps#35 We'll see where it goes..

In terms of other options:

@chris48s
Copy link
Member Author

chris48s commented Jan 4, 2018

I have no objection, but also don't have time to write the change myself. If you're able to write a PR, it has a nice better chance of happening.

Not sure how your Haskell is.. Perhaps if the issue was "/feed endpoint should calculate Nth fibonacci number" I might have a chance 😀

I reckon its time for Plan B.

@paulmelnikow
Copy link
Member

I'd love to give this an hour since it sounds like a fun challenge, though I'll resist the temptation. 😀

Worth leaving the issue open in case someone comes along to address it. Meanwhile two requests seems a fine way to handle this. Perhaps we could make the second request to one of his other routes. That way we know the data is in sync between the two checks.

e.g.

http://packdeps.haskellers.com/licenses/AbortT-mtl <- exists, 200
http://packdeps.haskellers.com/licenses/AbortT-mtlz <- doesn't exist, 404

@chris48s
Copy link
Member Author

chris48s commented Jan 6, 2018

sorted :)

@paulmelnikow
Copy link
Member

Looks great!

One last change I'd suggest while you're in here: change hackage-deps to dependencies for consistency with the other dependency badges.

@chris48s
Copy link
Member Author

chris48s commented Jan 6, 2018

Test failure is unrelated to this PR

floatdrop/pinkie-promise#4

pinkie-promise is a transitive dependency of other packages so this will probably take a while to settle out. Also note there may be some squatting going on: floatdrop/pinkie-promise#4 (comment)

@paulmelnikow paulmelnikow merged commit 2de53ce into badges:master Jan 7, 2018
@paulmelnikow
Copy link
Member

This came out looking really good. Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants