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

DataObjStr permits to set invalid priority #147

Closed
cblegare opened this issue Nov 6, 2020 · 10 comments · Fixed by #153
Closed

DataObjStr permits to set invalid priority #147

cblegare opened this issue Nov 6, 2020 · 10 comments · Fixed by #153
Milestone

Comments

@cblegare
Copy link

cblegare commented Nov 6, 2020

While generating a objects.inv from Mozilla MDN reference, I reused their "priority" settings to generate DataObjStr objects (usually 0.5), which is invalid as per https://sphobjinv.readthedocs.io/en/latest/syntax.html?highlight=priority#sphinx-objects-inv-v2-syntax

There is no such validation in the data model, and that could be a nice addition :)

@bskinn
Copy link
Owner

bskinn commented Nov 6, 2020

Thanks for submitting this -- I did actually think about adding this sort of validation to the DataObj classes, early on.

However, I don't think Sphinx uses priority when constructing cross-references, especially via intersphinx -- as best I can tell, the value is only used by Sphinx in the process of building docs, and by the time it's exported into objects.inv, it's not actually used further. So, I decided against any particular validation on it, since there may be use cases where arbitrary priority values might be desirable, for purposes other than just use with intersphinx.

But: Are you seeing a particular bug as a result of this? If there is a functional reason why priority should be constrained, then definitely I'd want to add validation for it.

@bskinn bskinn added type: enhancement ✨ Something to add issue: maybe 🤔 Being considered, but not certain labels Nov 6, 2020
@cblegare
Copy link
Author

cblegare commented Nov 6, 2020

Thank you for your feedback!

I indeed see a bug since the sphobjinv.Inventory could not import my objects.inv file. If you choose to allow arbitrary priorities, you might want to change the regex of sphobjinv.re.ptn_data to allow it too ;)

@bskinn bskinn added type: bug 🪲 Something is wrong pr: needs tests ✔️ and removed type: enhancement ✨ Something to add issue: maybe 🤔 Being considered, but not certain labels Nov 6, 2020
@bskinn bskinn added this to the v2.1 milestone Nov 6, 2020
@bskinn
Copy link
Owner

bskinn commented Nov 6, 2020

Ahh, indeed, I've restricted it to integers, haven't I!

Can you supply me with your objects.inv, so that I can use it for testing?

Needs:

  • Implementation change
    • Modified version of regex pattern here, with any sign
    • Allow any non-empty, non-whitespace-containing string
  • Add description of valid priority values to docs, and motivation for making priority general
  • Tests
    • Valid priority values
    • Invalid priority values
    • Consider hypothesis for this
    • Addition of the objects.inv with 0.5 priority values should suffice, mostly
    • Add test for inventory with non-numeric priority, including the sphinx_load_test

 

Thank you for reporting this!

@cblegare
Copy link
Author

cblegare commented Nov 6, 2020

Yes sure! I'll upload it here for now, but it might be available online quite soon

Here is a plain text version of it

objects_mdn.inv.txt

@bskinn
Copy link
Owner

bskinn commented Nov 6, 2020

Ah, that's a working version of the mdn inventory... would it be possible for you to generate a version that soi.Inventory can't load, but that you would like it to be able to? (E.g., with the 0.5 values for priority you mentioned.)

@cblegare
Copy link
Author

cblegare commented Nov 6, 2020

Yes sorry about that

objects_mdn_notworking.inv.txt

@bskinn
Copy link
Owner

bskinn commented Nov 10, 2020

For documentation purposes: Sphinx does only use the priority (prio within Sphinx) string value as a numeric value, in both the Python and JS search code.

However, these uses are only relevant to HTML documentation built by Sphinxintersphinx doesn't use the priority value at all—and so it seems like there's little reason to constrain objects.inv files created de novo to numerical priority values; it should be fine for them to be any non-empty string value containing no whitespace.

bskinn added a commit that referenced this issue Nov 10, 2020
Also add custom MDN inventory with non-integer priority values.

Fixes #147
@bskinn
Copy link
Owner

bskinn commented Nov 13, 2020

I'll try to get a prerelease of this fix up onto PyPI sometime today (US Eastern).

@bskinn
Copy link
Owner

bskinn commented Nov 13, 2020

Done. Give it a shot with pip install --pre, let me know how it goes.

@bskinn
Copy link
Owner

bskinn commented Feb 4, 2021

Per #181 (comment), the relaxation of accepted priority values is incompatible with the regex Sphinx uses when loading objects.inv files. This will lead to the object entries being silently ignored by Sphinx, and thus in turn lead to broken cross-references.

I'm going to have to revert the priority change for v2.1. Rigorous validation (#152) is a breaking change, so that won't happen until a v3.0 release. Don't know what the timeline will be for that yet... probably not any time soon.

@bskinn bskinn closed this as completed Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants