Skip to content

Simplify translation of terms#12

Merged
UnniKohonen merged 1 commit intoupdate-2022from
issue10-simplify-translation
Jan 25, 2023
Merged

Simplify translation of terms#12
UnniKohonen merged 1 commit intoupdate-2022from
issue10-simplify-translation

Conversation

@UnniKohonen
Copy link
Copy Markdown
Contributor

@UnniKohonen UnniKohonen commented Dec 30, 2022

This PR simplifies translation of suggested terms by using the language parameter of Annif API's suggest method. Calls to Finto API are removed.

Implements #10

@UnniKohonen UnniKohonen added the enhancement New feature or request label Dec 30, 2022
osma
osma approved these changes Jan 23, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Looks very good - we will get rid of a lot of code - and seems to work well.

I'm not sure what is the best merging strategy here (should we merge directly to main or to some other branch? @juhoinkinen ?) but once we figure that out, this can go in.

@juhoinkinen
Copy link
Copy Markdown
Member

juhoinkinen commented Jan 23, 2023

Hmm, I think it's best to merge to update-2022, because that branch is deployed for ai.dev.finto.fi , and main branch for ai.finto.fi.

@osma osma changed the base branch from main to update-2022 January 23, 2023 15:26
@osma
Copy link
Copy Markdown
Member

osma commented Jan 23, 2023

I changed the base branch, but now we have many more commits in this PR. I think there should be a merge from main to update-2022 first that would take care of the commits that are not yet in update-2022.

@juhoinkinen
Copy link
Copy Markdown
Member

juhoinkinen commented Jan 23, 2023

I changed the base branch, but now we have many more commits in this PR. I think there should be a merge from main to update-2022 first that would take care of the commits that are not yet in update-2022.

Ok, I'll merge on my laptop and push.

@juhoinkinen juhoinkinen changed the base branch from update-2022 to main January 23, 2023 15:43
@juhoinkinen juhoinkinen changed the base branch from main to update-2022 January 23, 2023 15:43
@juhoinkinen
Copy link
Copy Markdown
Member

juhoinkinen commented Jan 23, 2023

To make "the extra commits" disappear from thia PR, I also had to switch the base branch to main (any other branch could have done) and back to update-2022. This was suggested in StackOverflow.

osma
osma approved these changes Jan 24, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Can be merged now IMHO

@UnniKohonen UnniKohonen marked this pull request as ready for review January 25, 2023 11:15
@UnniKohonen UnniKohonen merged commit d9e9aea into update-2022 Jan 25, 2023
@UnniKohonen UnniKohonen deleted the issue10-simplify-translation branch January 25, 2023 11:17
@osma
Copy link
Copy Markdown
Member

osma commented Jan 25, 2023

🎉

@juhoinkinen juhoinkinen mentioned this pull request Mar 8, 2023
Closed
@osma osma mentioned this pull request Nov 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@osma osma osma approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@UnniKohonen @juhoinkinen @osma