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 #454 #550

Merged
merged 1 commit into from
Jan 18, 2017
Merged

Fix #454 #550

merged 1 commit into from
Jan 18, 2017

Conversation

jhnsmth
Copy link
Contributor

@jhnsmth jhnsmth commented Jan 16, 2017

Fix #454. Ported from here. Not sure if put the test into the right place (or whether it needed at all)

@lihaoyi
Copy link
Member

lihaoyi commented Jan 16, 2017

Looks like this makes CI mad as it can't find Akka configs. I've tried re-starting those jobs and they fail with the same error, so probably not just flakes. Akka loads the default config through resources IIRC

@jhnsmth
Copy link
Contributor Author

jhnsmth commented Jan 16, 2017

@lihaoyi Thanks for the quick reply. Didn't see them , when run the tests locally, because akka project test aren't run for 2.12. I think it the second overridden function is guilty for that. I'll let you know , when I fix it.

@jhnsmth jhnsmth changed the title Fix #454 (WIP) Fix #454 Jan 16, 2017
@jhnsmth
Copy link
Contributor Author

jhnsmth commented Jan 17, 2017

Travis doesn't like me, two build failed complaining about github API rate limit. Looks like this failures aren't related to my changes and failed akka test is green now, so please review.

BTW Since those tests randomly fail, maybe it makes sense to replace github api with something else or even to remove them.

@jhnsmth jhnsmth changed the title (WIP) Fix #454 Fix #454 Jan 17, 2017
@lihaoyi
Copy link
Member

lihaoyi commented Jan 17, 2017

Yeah I just ignore the rate limited tests. They suck but they'll do until someone gets around to fixing/removing them. Feel free to send a PR to replace that API endpoint with some other


private def getURLFromFileDict(name: String) = {
val className = name.stripSuffix(".class").replace('/', '.')
newFileDict.find(_._1 == className) map { c =>
Copy link
Member

Choose a reason for hiding this comment

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

Could this be replaced by newFileDict.get(className)? That will let it be O(1) rather than O(n) as the size of newFileDict grows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@lihaoyi
Copy link
Member

lihaoyi commented Jan 18, 2017

It's not easy being geen

@lihaoyi lihaoyi merged commit a0d5f97 into com-lihaoyi:master Jan 18, 2017
@lihaoyi
Copy link
Member

lihaoyi commented Jan 18, 2017

This should be published in the next unstable version if CI doesn't flake

@jhnsmth jhnsmth deleted the fix/454 branch January 18, 2017 09:29
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