Skip to content

Conversation

@Zalfsten
Copy link
Contributor

@julian-schneider , these changes were flagged as critical by a gemini code review. Please check them out.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @Zalfsten, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the ID handling logic within the publiso_conversor.jq script to improve its robustness and consistency. The changes ensure that IDs are generated reliably by explicitly checking for the existence of source ID fields before attempting to construct URLs, thereby preventing the creation of malformed or incomplete identifiers in the converted JSON output.

Highlights

  • Robust ID Generation: The primary @id field generation logic has been enhanced to explicitly check for the presence of a doi first, then an @id field, defaulting to null if neither is found. This prevents errors when expected ID fields are missing.
  • Conditional PropertyValue Creation: The PropertyValue object for frl-internal is now only created if the @id field exists in the input, ensuring that incomplete or erroneous property values are not generated.
  • Safer DataDownload ID Handling: Within the distribution array, the @id for DataDownload objects is now conditionally generated, checking for the presence of @id in each hasPart item before constructing its URL, falling back to null if the ID is absent.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • middleware/scripts/publiso_conversor.jq
    • Refactored the top-level @id generation to prioritize doi, then @id, and default to null if neither is present.
    • Modified the PropertyValue object creation for frl-internal to be conditional on the existence of the @id field.
    • Updated the DataDownload @id generation within the distribution array to conditionally check for the @id field.
Activity
  • The changes were flagged as critical by a Gemini code review, indicating a prior analysis and recommendation for these improvements.
  • The pull request author, Zalfsten, has requested a review from @julian-schneider.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves the robustness of the jq conversion script by adding checks for the existence of the @id field before its use. This prevents potential runtime errors and ensures that URLs are generated correctly, which is a critical fix. The changes are well-implemented. I've provided a couple of minor suggestions to refactor the new code to be more concise and idiomatic to jq by using pipes to avoid repeating field accessors.

@julian-schneider
Copy link
Contributor

Hey, thanks for looking into this stuff. I gave my quick opinion about the AI comments in the old PR already: I'm completely fine if you merge its suggested changes. I have also executed this script with and without these changes, and both versions produced the same output.

More details: This PR adds a fallback for a situation where @id is not present. Previously, the jq script wouldn't be able to parse such an input and quit with no output file. With these code changes, the jq script will instead produce an output file with "@id": null, which will be rejected by the Search Hub because the identifier is required. So from the Search Hub side, both cases are equivalent. Feel free to merge this, if the second outcome would make your life easier.

In case you're wondering, this situation currently doesn't occur because FRL datasets always have an @id.

@Zalfsten
Copy link
Contributor Author

Thanks, @julian-schneider .
I've expected your reply.
I'm not sure, how to deal with this. In "real" code I would like to throw an exception if a prerequisite (like "@id has to be specified") isn't met. Probably I would introduce an Exception class like PrerequisiteNotMet. In this case the jq run fails with a more or less meaningful error message as string. A message that currently nobody will perceive, as the logs are never inspected.
On the other hand, it's usually best practice so fail early...
I tend not to merge this pull request...

@julian-schneider
Copy link
Contributor

I researched this, and it looks like jq can exit with a specific error message: https://jqlang.org/manual/#error

I suggested an example usage to a relevant line

@Zalfsten
Copy link
Contributor Author

Ok, I've introduced your suggestion, @julian-schneider , as well as the two minor gemini suggestions. Can I check on my own if it works?

@julian-schneider
Copy link
Contributor

I suppose it would be best to run this repo's pipeline to see if it works. But if you have jq installed locally you could quickly run the script in isolation with such a shell script:

#!/bin/sh

jq -f publiso_conversor.jq publisso.json > temp.json
jq -s '.' temp.json > frlSchemaMetadata.json
rm temp.json

where "publisso.json" is the json response from the FRL API

I've executed it just now and the result is still the same 👍

@Zalfsten Zalfsten merged commit 6d6f7a1 into main Feb 11, 2026
5 checks passed
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.

2 participants