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

Make translation directory locations modular #5380

Merged
merged 35 commits into from
Oct 25, 2022
Merged

Make translation directory locations modular #5380

merged 35 commits into from
Oct 25, 2022

Conversation

mattjdnv
Copy link
Contributor

@mattjdnv mattjdnv commented Jun 30, 2022

Added code to the translationServer to use a hoot config variable for translation directory locations

Adding to hoot-services next. Am trying to have one way to specify translation directories at run time to make deployments easier.

Got rid of the config var and now it searches $HOOT_HOME/translations* looking for translationConfig.json files
This file is a combination of the DefaultTranslations.json file and the TranslationServer config file.

There is a bit of cleanup to do to remove the last of the hardcoded translations-local stuff.
And, as usual, the documentation needs an update as well.

@mattjdnv mattjdnv self-assigned this Jun 30, 2022
@mattjdnv mattjdnv changed the title Making translation directory location modular Make translation directory locations modular Jun 30, 2022
Conflicts:
	hoot-js/src/main/cpp/hoot/js/util/RequireJs.cpp
	translations/TranslationServer.js
	translations/checkAllFeatures.js
This will be easier to maintain in the future.  Now to fix the UI

refs: #5379
Copy link
Contributor

@brianhatchl brianhatchl left a comment

Choose a reason for hiding this comment

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

Was it intentional to change the order of the translations in the schema switcher drop down?
was this:
Screenshot from 2022-10-24 13-40-27

now this:
Screenshot from 2022-10-24 12-41-30

This change seems like it was intended to "demote" the earlier versions of TDS in importance?

@mattjdnv
Copy link
Contributor Author

Yes. I did re-order the translation list to put the newer / more popular ones first.

@brianhatchl
Copy link
Contributor

Not sure why this same core test is failing:

Failure: N4hoot19AddressConflateTestE::runTest

src/test/cpp/hoot/core/conflate/address/AddressConflateTest.cpp(185)   - Expected: 21

- Actual  : 28

It seems to pass locally when run in isolation with:

HootTest --single N4hoot19AddressConflateTestE::runTest

Copy link
Contributor

@brianhatchl brianhatchl left a comment

Choose a reason for hiding this comment

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

Not sure what to do about the test failure

@bmarchant
Copy link
Contributor

This looks like a change that happened with the upgrade of the libpostal library. I'll take a look.

@brianhatchl
Copy link
Contributor

ok, I definitely had not re-provisioned since that update

@bmarchant
Copy link
Contributor

@mattjdnv BLUF: if you merge origin/master into this branch, it should fix that error. (or you can ignore it, merge this PR and everything will be in master and it will be fine.)

Long version: Because libpostal was upgraded in hoot-deps from 1.0 to 1.1 and we weren't locking the version. If you look at the provisioning you've got v1.1 installed but you don't have the updated unit tests from this PR to make them pass. So like I said, if you run git merge origin/master on your local branch and re-push, this unit test will pass. Or you can merge this PR and since all of the other unit tests passed, it should mean everything in master will pass too. Up to you.

@brianhatchl brianhatchl merged commit e0e9c3f into master Oct 25, 2022
@brianhatchl brianhatchl deleted the 5379 branch October 25, 2022 17:54
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.

3 participants