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

Issue251 cli init command #306

Merged
merged 10 commits into from
Aug 23, 2019
Merged

Issue251 cli init command #306

merged 10 commits into from
Aug 23, 2019

Conversation

juhoinkinen
Copy link
Member

The init command simply deletes all files under data/projects/project-to-remove/ (that directory will remain):

$ annif init tfidf-fi-copy 
Removed data files for project tfidf-fi-copy.

$ annif init nonexistent-project
No projects found with id 'nonexistent-project'.

Stated in the issue comment: "It could wipe all state and reset any configuration settings."
Is this PR enough for this?

The issue #269 for creating backend for using Maui server plans using init command for "creating tagger on the Maui server end", which is left for future.

This closes #251.

@juhoinkinen juhoinkinen marked this pull request as ready for review August 12, 2019 09:59
@codecov
Copy link

codecov bot commented Aug 12, 2019

Codecov Report

Merging #306 into master will increase coverage by 19.55%.
The diff coverage is 95.23%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #306       +/-   ##
===========================================
+ Coverage    79.8%   99.35%   +19.55%     
===========================================
  Files          55       55               
  Lines        2842     2962      +120     
===========================================
+ Hits         2268     2943      +675     
+ Misses        574       19      -555
Impacted Files Coverage Δ
annif/cli.py 99.48% <100%> (+0.01%) ⬆️
tests/test_cli.py 100% <100%> (ø) ⬆️
annif/project.py 99.41% <83.33%> (+0.62%) ⬆️
annif/corpus/subject.py 100% <0%> (ø) ⬆️
annif/corpus/types.py 100% <0%> (ø) ⬆️
annif/corpus/document.py 100% <0%> (ø) ⬆️
annif/corpus/convert.py 100% <0%> (ø) ⬆️
tests/test_corpus.py 100% <0%> (ø) ⬆️
annif/analyzer/__init__.py 92.59% <0%> (+3.7%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 252c87e...fa21be3. Read the comment docs.

annif/cli.py Outdated Show resolved Hide resolved
annif/cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Show resolved Hide resolved
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

It's very good now but I'm still thinking about what the semantics should be here.

Originall I thought that this command could also be used to initialize Maui Server (once we add support for it) but I'm not so sure anymore - maybe that's overloading the command too much. It would be simpler to just define this command as clearing the state of a project - as is currently implemented. Consequently it would be best to call the command annif clear instead of annif init.

So just two things needed before this can be merged:

  1. Rename the command to clear
  2. Show a warning instead of an error if there is nothing to remove.

Sorry for dithering on this!

annif/project.py Outdated
"""remove the data of this project"""
datadir_path = self._datadir_path
if not os.path.isdir(datadir_path):
raise AnnifException('No model data to remove for project {}.'
Copy link
Member

Choose a reason for hiding this comment

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

I think this should emit a warning message but it shouldn't be a fatal error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented showing warning instead of erroring.

In the test for nonexistent data the command is run for dummy-fi project, because when running for a nonexistent project a (different) warning is raised already in get_project.

The test has logger.propagate = True for the same reason as in #299 (comment).

@osma osma merged commit d2dff10 into master Aug 23, 2019
@osma
Copy link
Member

osma commented Aug 23, 2019

Still need to document this on the wiki page Commands

@juhoinkinen juhoinkinen added this to the 0.42 milestone Sep 2, 2019
@juhoinkinen juhoinkinen deleted the issue251-CLI-init-command branch September 25, 2019 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI init command for clearing project data
2 participants