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

refactor: cleanup #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wadjih-bencheikh18
Copy link
Member

No description provided.

@stropitek stropitek assigned stropitek and unassigned stropitek Mar 2, 2022
@stropitek stropitek self-requested a review March 2, 2022 12:21
Copy link
Member

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

Please restore the .docs/Readme.hbs

It is used as a template to generate the readme. It's a bit strange that it is in a hidden folder, you can change that.

Please check that the generation is still working (npm run docs) after re-installing the deps.

@targos do you know why the nodejs workflow isn't running in this PR?

@targos
Copy link
Member

targos commented Mar 2, 2022

do you know why the nodejs workflow isn't running in this PR?

No idea. I verified that actions are enabled for the repo.

@targos
Copy link
Member

targos commented Mar 2, 2022

Btw I would be surprised if everything still works without changes given the huge amount of major dependency upgrades.

@wadjih-bencheikh18
Copy link
Member Author

No idea. I verified that actions are enabled for the repo.

Going to check what's the problem

@stropitek
Copy link
Member

stropitek commented Mar 2, 2022

Yes I think the dependencies should not be bumped in this one. There is an ongoing branch which updates some of the deps, so there will be conflicts

https://github.com/mljs/libsvm/tree/upgrade-deps

I'm working on this on my free time but I think that branch can be merged once I find a few hours to finish it.

@stropitek
Copy link
Member

Also the tests rely on code that is generated by emscripten, so no chance this will work without a build step first

@targos
Copy link
Member

targos commented Mar 2, 2022

I think we should hold off on this PR. Finish your branch first, then work on this again (emscripten will have to be installed in the workflow somehow)

@targos targos added the blocked label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants