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

Add lookup tables as git submodules #1902

Merged
merged 8 commits into from
Sep 25, 2023
Merged

Add lookup tables as git submodules #1902

merged 8 commits into from
Sep 25, 2023

Conversation

dr0i
Copy link
Member

@dr0i dr0i commented Sep 22, 2023

Do [edited:] git config --local submodule.recurse true once in your checked out repo. Any further git pull will automatically update the submodules.

See #1893.

Do "git config --global submodule.recurse true" once in your checked out repo.
Further "git pull" will automatically update the submodule.
Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

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

Do "git config --global submodule.recurse true" once in your checked out repo.

I don't think that's such a good recommendation. I'd suggest --local instead (which is the default). Or just use git pull --recurse-submodules explicitly.

@dr0i
Copy link
Member Author

dr0i commented Sep 25, 2023

--local is good. (I like to use the "established" commands (here git pull) to work with the new setting - not only because of cronjobs and scripts (which will work without adapting them).

@TobiasNx
Copy link
Contributor

src/test/resources/deweyLabels.tsv should also be moved into the maps folder.

@TobiasNx
Copy link
Contributor

Also we do not need a specific src/test/resources/alma/maps folder a folder src/test/resources/maps would be good enough.

@blackwinter suggested to use gitsubmodules to easily be in sync
with our repos containing lookup tables.
@dr0i
Copy link
Member Author

dr0i commented Sep 25, 2023

We have src/main/resources/alma/maps/ and thus src/test/resources/alma/maps/ is symmetric - which is a very nice property and should be kept.

Re dewey: done that
Re further discussion of where to put test resources: that's not the scope of this PR/issue so we should open a new one if needed.

Copy link
Contributor

@TobiasNx TobiasNx left a comment

Choose a reason for hiding this comment

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

Two further questions:

Shouldn't we delete the old lookup file in the old maps folder that are now used with the submodules?
Also shouldn't we document in the maps folder, that for certain maps the submodules are used, not just in the over-all README.md

Otherwise +1

@dr0i
Copy link
Member Author

dr0i commented Sep 25, 2023

I've deleted now src/main/resources/alma/maps/generatedAlmaSuppressedLocations.tsv.
Re documentation: I don't understand why we should put a readme into the maps directory when these lookup tables are not read from this directory.

@dr0i dr0i assigned TobiasNx and unassigned dr0i Sep 25, 2023
@TobiasNx
Copy link
Contributor

I've deleted now src/main/resources/alma/maps/generatedAlmaSuppressedLocations.tsv. Re documentation: I don't understand why we should put a readme into the maps directory when these lookup tables are not read from this directory.

okay, then go for it.
I just thought that it helps to find missing map files when somebody does not find it in the folder. But then go on. +1

README.textile Outdated Show resolved Hide resolved
@dr0i dr0i merged commit 078eac9 into master Sep 25, 2023
1 check passed
@dr0i dr0i deleted the 1893-addSubmodules branch September 25, 2023 14:34
@dr0i
Copy link
Member Author

dr0i commented Oct 2, 2023

Unfortunatley it seem not possible to have the submodules not in a detached state (may depend on the git version) but you have to do git submodule update --recursive --remote after git pull. Will update all scripts and README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants