search

nestauk / ojd_daps_skills

Nesta's Skills Extractor Library
https://nestauk.github.io/ojd_daps_skills
MIT License
123 stars 19 forks source link

Windows Related Changes #183

Closed Aiden-RC closed 1 year ago

Aiden-RC commented 1 year ago

Thanks for contributing to Nesta's Skills Extractor Library 🙏!

If you have suggested changes to code anywhere outside of the ExtractSkills class, please consult the checklist below.

lizgzil commented 1 year ago

thanks so much for this @Aiden-RC ! I'm wondering why our test checks haven't been triggered - perhaps because this PR is from a forked dev branch, so changes haven't been pushed to a branch? Would it be possible to create a branch from dev with these pushed changes instead (unless you have another idea)? We would need the checks to pass before merging you see. Thanks!

Aiden-RC commented 1 year ago

thanks so much for this @Aiden-RC ! I'm wondering why our test checks haven't been triggered - perhaps because this PR is from a forked dev branch, so changes haven't been pushed to a branch? Would it be possible to create a branch from dev with these pushed changes instead (unless you have another idea)? We would need the checks to pass before merging you see. Thanks!

Ah yeah that is weird, i'll look into your checks and see what i can do

Aiden-RC commented 1 year ago

thanks so much for this @Aiden-RC ! I'm wondering why our test checks haven't been triggered - perhaps because this PR is from a forked dev branch, so changes haven't been pushed to a branch? Would it be possible to create a branch from dev with these pushed changes instead (unless you have another idea)? We would need the checks to pass before merging you see. Thanks!

so after a quick check, your UnitTests are setup on push to branch, since the code changes i did were on a fork your unit tests wont run until you merge them, however the unit tests will run on my fork, so heres a link to that https://github.com/Aiden-RC/ojd_daps_skills/actions/runs/4177005446

Also i noticed whilst running the unit tests that the en-core-web-sm version being downloaded and installed is incomptible with the version of spacy you've selected in the requirements image

Aiden-RC commented 1 year ago

if you'd like the unit tests to run on your project rather than mine ( for peace of mind ), you can create a new branch, re assign this PR to point into that branch, merge the PR and that should trigger your checks, then once they pass you can merge from the new branch into dev :)

Aiden-RC commented 1 year ago

thanks so much for this @Aiden-RC ! I'm wondering why our test checks haven't been triggered - perhaps because this PR is from a forked dev branch, so changes haven't been pushed to a branch? Would it be possible to create a branch from dev with these pushed changes instead (unless you have another idea)? We would need the checks to pass before merging you see. Thanks!

oh also i committed a change to the workflow which will ( once merged ) trigger a check on push and PR which will trigger the action from forks aswell 👍

Checks ran for that commit are here: https://github.com/Aiden-RC/ojd_daps_skills/actions/runs/4177069276

lizgzil commented 1 year ago

Merging into https://github.com/nestauk/ojd_daps_skills/pull/184 due to tests not triggering here

lizgzil commented 1 year ago

thanks @Aiden-RC I've merged these into https://github.com/nestauk/ojd_daps_skills/pull/184/files and the tests are currently running (it's a shame the triggering doesn't work for push OR pull request since now 2 sets of the same test need to run). Thanks for pointing out the spacy issue too - I'll look into this. I think in another repo we had to pin en_core_web_sm to 3.4.1