Closed abitrolly closed 2 years ago
This looks really nice. Do you mind cleaning the commits a bit, remove the debugging one and squash ones that are basically fixing bugs in the changes?
I merged #212 .
@idank squashed a bit. Installation and linting works without problems, but tests fail for some reason. Maybe because I had to upgrade NLTK, I don't know.
nosetests --with-doctest tests/ explainshell/
....F...........................................................................
======================================================================
FAIL: test_aliases (tests.test-manager.test_manager)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/explainshell/explainshell/tests/test-manager.py", line 70, in test_aliases
self.assertEquals(len(mp), 2)
AssertionError: 1 != 2
Let's skip those tests then. I don't think they're that important since we're not using that code.
@idank all green now - https://github.com/abitrolly/explainshell/actions/runs/3213172763
Thanks! The commit that bumps nltk can be removed since I merged https://github.com/idank/explainshell/pull/244, do you mind fixing that?
Also if it's not too much trouble can you change the commit summary to follow the style of subject: summary
, e.g. tests: integrate with github actions
.
Yup. Lemme rebase that.
@idank done!
Merged, thanks! :)
There are a couple necessary fixes with dependencies. Basically it my experience takes a few hours to setup the project and report one of the issues.
The tests fail with this problem.
Which can be repeated without the test suite.
This fix is proposed in https://github.com/idank/explainshell/pull/212 but I am not proficient in Perl to review it.
Merging this PR will at least allow to see if other PRs do good without executing them locally.