Skip to content

Conversation

@ArnaudPel
Copy link
Contributor

Fixes #1

Changes proposed in this pull request:

  • Fix implementation of longestCommonSubstring

@burgaard

@ArnaudPel
Copy link
Contributor Author

At your disposal to give more details on what issue this change is (hopefully) fixing, if needs be.

@ArnaudPel
Copy link
Contributor Author

PS: the expected check seems to be stale, which seems to be a known issue:
https://github.community/t5/GitHub-Actions/Expected-Waiting-for-status-to-be-reported/td-p/36575

@burgaard
Copy link
Owner

Hi @ArnaudPel,

Wow, thank you so much for both discovering the issue and then taking the effort to implement a fix! It looks like something happened to my Travis-CI integration. I'll take a look at that tonight and review your pull request.

Cheers,
Kim

@ArnaudPel
Copy link
Contributor Author

Thanks to the lockdown in France, I've got more spare time, so it was a real pleasure to eventually be able to pin down the issue in the implementation. Incidentally, I now have a better idea of the way the algorithm works :)

Thank you for having publish that lib BTW, I'll use the fixed version in our app when it is released on npm.

@ArnaudPel
Copy link
Contributor Author

Hello, could you please let me know if you plan to release this fix soon ? Otherwise I'll have to generate a temporary package myself, for our internal use.

As I offered, I've still got the debugging data that helped me find the fix, so I believe I can come up with a somewhat visual explanation, if needs be.

Thanks

@burgaard
Copy link
Owner

@ArnaudPel sorry, my work is blessed to be one of the few kinds of businesses that have seen an uptick since novel coronavirus happened. I'll make an effort to test and merge your PR by end of Thursday Pacific time.

@burgaard
Copy link
Owner

Regarding the build check being stuck, what happened was my OAuth token between Travis CI and GitHub had expired.

@burgaard
Copy link
Owner

@ArnaudPel derp, I restarted the build in Travis, but the coveralls integration appears to not like that:

/home/travis/build/burgaard/string-algorithms/node_modules/coveralls/bin/coveralls.js:18
232        throw err;
233        ^
234Bad response: 422 {"message":"service_job_id (675690138) must be unique for Travis Jobs not supplying a Coveralls Repo Token","error":true}

Would you mind pushing an empty commit to see if that's a quick way to get past that?

Copy link
Owner

@burgaard burgaard left a comment

Choose a reason for hiding this comment

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

🎉

@burgaard burgaard merged commit ea4d726 into burgaard:master Apr 30, 2020
@ArnaudPel ArnaudPel deleted the fix-longest-common-substring branch April 30, 2020 08:45
@burgaard
Copy link
Owner

@ArnaudPel version 1.0.21 with your changes has been published to npm. Sorry for the delay -- it's been about two years since I made the last update and in the meanwhile all the tokens to connect Travis CI, GitHub and npm had expired.

ArnaudPel pushed a commit to abolis-biotechnologies/plate-maker that referenced this pull request May 4, 2020
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.

Erroneous result in longestCommonSubstring

2 participants