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

lookup formangabe #2090

Closed
wants to merge 4 commits into from
Closed

lookup formangabe #2090

wants to merge 4 commits into from

Conversation

maipet
Copy link
Contributor

@maipet maipet commented Nov 5, 2024

Related to #2082

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.

Fixes look good. But I would change the commits a bit so that you can better see the changes that your code introduces.

Add the test records from 1fed759 in an commit before adding the changes in the code.
In a second commit add the changes in the fixes as well as the changes that result when running: ~/git/lobid-resources$ mvn clean install -DskipTests=false -DgenerateTestData=true. You can also fix the formatting in this commit.

In a third commit add http://lobid.org/resources/99374153235806441 as test for fixing the columns of the formangaben lookup.

In a fourth commit add your changes to the fix from: bb77c45 as well as the changes that result when running: ~/git/lobid-resources$ mvn clean install -DskipTests=false -DgenerateTestData=true.

In a last commit update the web app tests or you could add the updates of the web app tests when introducing a new test. (Which I usually forget)

@maipet
Copy link
Contributor Author

maipet commented Nov 5, 2024

@TobiasNx Thx for your suggestions. As the JSON was not created due to the incorrect transformation, it is not possible to see the changes between two results. I have therefore edited this in one step. Where does the example http://lobid.org/resources/99374153235806441 come from? Sorry, but I cannot see any relation to #2082.

@TobiasNx
Copy link
Contributor

TobiasNx commented Nov 5, 2024

@TobiasNx Thx for your suggestions. As the JSON was not created due to the incorrect transformation, it is not possible to see the changes between two results. I have therefore edited this in one step. Where does the example http://lobid.org/resources/99374153235806441 come from? Sorry, but I cannot see any relation to #2082.

The example came up in context of #2028 and was provided by you having a wrong publication date. But the JSON Schema Tests og the github action failed due to the mapped natureOfContent.id being wrong: https://github.com/hbz/lobid-resources/actions/runs/11680572050/job/32523855291#step:8:187

It will be fixed by your commit: bb77c45 Therefore it would be a good test file for this commit.

@TobiasNx TobiasNx mentioned this pull request Nov 5, 2024
@maipet
Copy link
Contributor Author

maipet commented Nov 6, 2024

replaced by #2092

@maipet maipet closed this Nov 6, 2024
@maipet maipet deleted the 2082-LookupFormangabe branch November 6, 2024 09:51
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.

2 participants