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

Issue10 #29

Merged
merged 15 commits into from
Nov 3, 2017
Merged

Issue10 #29

merged 15 commits into from
Nov 3, 2017

Conversation

tvirolai
Copy link
Contributor

@tvirolai tvirolai commented Nov 3, 2017

This PR fixes issue 10.

It contains the following changes:

  • Flask has been upgraded to Connexion
  • The API specification is mapped to the code and the Swagger UI works (although not all of the specified functionality is implemented yet).
  • The directory structure has been changed a little bit. The only way I could get the imports working everywhere was to move the stuff from the directory ./annif to the project root.
  • The annif.py file has also been refactored a bit. After struggling a little bit with how to call the functions both via the API and the command line, I separated these tasks to different functions. For example, the function show_project now returns JSON when called via the API. When called from the command line, the click command 'show-project' calls another a function that calls 'show_project' and parses the response to a human-readable string. This works and I think this separation of concerns makes testing easier in the long run. Dunno how elegant a solution this is.

@tvirolai tvirolai requested a review from osma November 3, 2017 12:29
@osma
Copy link
Member

osma commented Nov 3, 2017

Looks good! It's definitely a big step forward so I think we can merge this.
I hope we can find a way to restore the directory structure so that we don't need to have annif.py and other Python scripts in the root directory, but it's OK for now.

@tvirolai tvirolai merged commit 0e052d4 into master Nov 3, 2017
@tvirolai tvirolai deleted the issue10 branch November 3, 2017 12:41
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.

None yet

2 participants