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

Update README.md #81

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Update README.md #81

merged 1 commit into from
Jan 29, 2024

Conversation

mattkeanny
Copy link
Contributor

Removed input format 'original' from example comment. Acc. to the OPS guide, section 2.1.2, the 'original' format is used only by the OPS number-service (section 3.3) hence only confusing in combination with the published_data() method.

@gsong
Copy link
Member

gsong commented Jan 27, 2024

@mattkeanny As far as I understand you can use any of the three numbering formats interchangeably, is there a disadvantage to calling out that you can use epo_ops.models.Original?

Or are you saying that published_data() won't work with an original number?

(Sorry, it's been a while since I've worked with EPO data, so I'm rusty).

@mattkeanny
Copy link
Contributor Author

@gsong Indeed published_data() does not work with an original number format, only with the docdb and epodoc.
In the OPS guide it is documented only in side-notes here and there (like in section 2.1.2).

Tested the ops api on the ops developers site using original as the input format just to confirm it (the the only service that works with the original format acc. to the ops guide is the Number Service):

/published-data/publication/original/EP.(1 000 000)/... -> fails
/number-service/publication/original/EP.(1 000 000)/... -> works

@amotl
Copy link
Member

amotl commented Jan 27, 2024

Hi Matt.

This is interesting, thanks for sharing, and for your patch. Now, I am thinking about whether the library should also respect this fact, and reject that operation by raising an appropriate exception educating the user correspondingly.

With kind regards,
Andreas.

NB: On the failing CI checks, I will try to submit a patch that will take into account that PRs submitted by external contributors do not have access to the credentials/secrets deposited at GHA, in this case OPS_KEY. Please do not merge this PR yet, as we can use it as an exercise to validate this improvement.

@mattkeanny
Copy link
Contributor Author

This is interesting, thanks for sharing, and for your patch. Now, I am thinking about whether the library should also respect this fact, and reject that operation by raising an appropriate exception educating the user correspondingly.

Makes sense indeed. Would spare a user pondering over the meaning of the returned 404 error.

@mattkeanny
Copy link
Contributor Author

@amotl: Skimming through the ops guide, section 4.5 reads:

As OPS provides access to data that is held inside the EPO, the OPS services (except
the number-service) require the input to be in one of the two EPO formats: epodoc or
docdb. The most common use case of the number-service is thus to transform numbers
that are given in domestic formats of different countries to the EPO formats that can
then be used for receiving published, family, register, legal or classification data.

which would tend to reinforce your suggestion to raise an early exception when using original in any method except the number() method.

@gsong
Copy link
Member

gsong commented Jan 27, 2024

@amotl: Skimming through the ops guide, section 4.5 reads:

As OPS provides access to data that is held inside the EPO, the OPS services (except
the number-service) require the input to be in one of the two EPO formats: epodoc or
docdb. The most common use case of the number-service is thus to transform numbers
that are given in domestic formats of different countries to the EPO formats that can
then be used for receiving published, family, register, legal or classification data.

which would tend to reinforce your suggestion to raise an early exception when using original in any method except the number() method.

Added #84.

@mattkeanny In this PR, would you be willing to change client method table https://github.com/ip-tools/python-epo-ops-client?tab=readme-ov-file#client to include a checkbox "Works with Original" column? Or something similar that conveys the spirit of this discussion?

@mattkeanny
Copy link
Contributor Author

@gsong I would prefer not to "overcrowd" the table, which to me looks succinct. I think this could go nicely into api docs and docstrings?

@gsong
Copy link
Member

gsong commented Jan 27, 2024

@gsong I would prefer not to "overcrowd" the table, which to me looks succinct. I think this could go nicely into api docs and docstrings?

That sounds reasonable. Feel free to tack on to the issue or add another issue.

@amotl
Copy link
Member

amotl commented Jan 28, 2024

Please do not merge this PR yet, as we can use it as an exercise to validate this improvement.

Hi. GH-86 has a corresponding patch. After merging it, and rebasing, this PR should also signal a successful outcome on CI.

Edit: Merged. Please rebase.

Removed input format 'original' from example comment.
Acc. to the OPS guide, section 2.1.2, the 'original' format is used only by the OPS number-service (section 3.3) hence only confusing in combination with the published_data() method.
@amotl
Copy link
Member

amotl commented Jan 29, 2024

I've rebased through GitHub UI, and will merge the patch now.

@amotl amotl merged commit bcee937 into ip-tools:main Jan 29, 2024
2 checks passed
@mattkeanny mattkeanny deleted the docs branch February 1, 2024 05:25
@CholoTook
Copy link
Contributor

@gsong I would prefer not to "overcrowd" the table, which to me looks succinct. I think this could go nicely into api docs and docstrings?

I'm so sorry, I'm coming at all this as an outsider... I'm literally searching issues to help me find documentation 😅

You mention the client method table being succinct, but I'm having a hard time understanding it.. Somewhere could you provide an example of how to call one of the rows?

Shall I create a separate issue for this ... issue?

Many thanks,

@mattkeanny
Copy link
Contributor Author

mattkeanny commented Apr 2, 2024

@gsong I would prefer not to "overcrowd" the table, which to me looks succinct. I think this could go nicely into api docs and docstrings?

I'm so sorry, I'm coming at all this as an outsider... I'm literally searching issues to help me find documentation 😅

You mention the client method table being succinct, but I'm having a hard time understanding it.. Somewhere could you provide an example of how to call one of the rows?

Shall I create a separate issue for this ... issue?

Many thanks,

@CholoTook The library is a wrapper around the OPS API, and the difficulty at present is that there is no tutorial or examples scripts to showcase the different methods and responses. So you'll have to refer back to the OPS reference guide most of the time. Perhaps you want to open a discussion item on a specific topic or an issue, whichever you think is appropriate?

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.

4 participants