Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

New JRC API #4

Merged
merged 8 commits into from
Jul 1, 2021
Merged

New JRC API #4

merged 8 commits into from
Jul 1, 2021

Conversation

kruzikh
Copy link
Contributor

@kruzikh kruzikh commented Jun 29, 2021

Update-rat is now consuming the new JRC API which provides information about RAT test being removed from the common list. Script feeds active attribute based on RAT database and writes date of removal of the test in the version attrbute. This is a slightly modified script created by Daniel Karlsson.

Update-rat is now consuming the new JRC API which provides information about RAT test being removed from the common list. Script feeds active attribute based on RAT database and writes date of removal of the test in the version attrbute. This is a slightly modified script created by Daniel Karlsson.
@kruzikh kruzikh requested review from dslmeinte and gabywh June 29, 2021 13:09
@dslmeinte
Copy link
Contributor

@kruzikh Could you put the lang and active fields directly after the display field in the jq expression? That keeps the diff in test-manf.json small.

@dslmeinte
Copy link
Contributor

@kruzikh Could you document the meaning of the version field in relation to the active field somewhere/somehow?

Also: the jq expression is quite non-trivial. Is it an option to implement this functionality in Python if it needs to perform more complicated functionality?

@gabywh
Copy link
Contributor

gabywh commented Jun 29, 2021

Also: the jq expression is quite non-trivial. Is it an option to implement this functionality in Python if it needs to perform more complicated functionality?

  1. If it's not trivial in jq, what makes you think it will be trivial in Python?
  2. Why re-write something that is delivered and already working and certainly clearly does seem to be maintainable (by at least two other people) in different teams and different countries?
  3. I thought you were in the eHN meeting today where we clearly stated SG-S define the content of the valuesets and for e.g. rat not a snapshot of the current JRC but the process of how to generate which is this jq script

@gabywh gabywh requested a review from ryanbnl June 29, 2021 17:53
gabywh
gabywh previously approved these changes Jun 29, 2021
@ryanbnl
Copy link
Contributor

ryanbnl commented Jun 30, 2021

I would also like to see the order of the fields in the output made consistent with the current version, and also something added to the readme.md in the root of the repository to document the script (another PR is fine).

Otherwise it's fine - it falls into the zone where shell scripts are effective; so IMO no need to rewrite it in a scripting language :)

kruzikh added 2 commits June 30, 2021 19:23
README added, script modified to hane order of elements as required.
ryanbnl
ryanbnl previously approved these changes Jun 30, 2021
@kruzikh kruzikh requested a review from gabywh June 30, 2021 17:29
@gabywh
Copy link
Contributor

gabywh commented Jun 30, 2021

Otherwise it's fine - it falls into the zone where shell scripts are effective; so IMO no need to rewrite it in a scripting language :)

If it ain't broke, don't fix it ;)

Copy link
Contributor

@gabywh gabywh left a comment

Choose a reason for hiding this comment

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

  1. decide on version - major (2.0.0), minor (1.4.0) or patch (1.3.1) - see https://semver.org/ for guidance if unsure
  2. CET -> CEST -> UTC?

kruzikh added 3 commits July 1, 2021 08:54
As information about insertion/removal of the item into/from HSC common list is provided only to date (no time), the script was changed and version element is now filled only with the date (of last change in JRC database or removal from HSC list).
@kruzikh kruzikh requested a review from gabywh July 1, 2021 08:59
@gabywh gabywh merged commit 46df494 into main Jul 1, 2021
@gabywh gabywh deleted the new-RAT-API branch July 1, 2021 11:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants