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

I314 numref hoverxref #315

Merged
merged 22 commits into from
May 26, 2020

Conversation

kerimoyle
Copy link
Collaborator

Closes #314

... but ... this currently is proof of concept as it requires a branch from the hoverxref repo. It can be merged now if we're desperate, or we can wait for it to be included in their update.

See change in the requirements file that would need to be reverted.

Rendered docs are here: https://cellml-specification-dev.readthedocs.io/en/i314_numref_hoverxref/reference/formal_and_informative/specB09.html

@kerimoyle kerimoyle requested review from agarny and hsorby April 20, 2020 20:27
@kerimoyle
Copy link
Collaborator Author

Hey @hsorby and @agarny ... am I ok to merge this? The fix has been made in the master repo of the hoverxref site now.

requirements.txt Outdated
@@ -1,3 +1,3 @@
sphinx>2,<3
sphinx-hoverxref
git+git://github.com/readthedocs/sphinx-hoverxref.git@master
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... not keen on this. master seems to be a wip type of branch. So, who knows, it may at some point contain something that could break things for us. You say that they have fixed the issue numref. Is that this commit: readthedocs/sphinx-hoverxref@3558a67? If so, and if anything, we might want to point to that one rather than master itself. However, my preference would on using an official version of sphinx-hoverxref.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's the commit. I've tried using just the plain sphinx-hoverxref requirement in the file, and it didn't seem to be building it in properly (either that or the cache clearing / readthedocs wiping wasn't being very thorough?). I'm not sure what the "official" sphinx-hoverxref would point to other than the master branch though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess I could ask when the next release will be (seems to be quite frequently) so then we can tie it to a tag - that's probably cleaner?

Copy link
Contributor

Choose a reason for hiding this comment

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

The official sphinx-hoverxref version is 0.3b1 (as confirmed by my local copy of sphinx-hoverxref and https://github.com/readthedocs/sphinx-hoverxref/releases). So, yes, ideally we would ask them for a new release with the numref fix and keep using sphinx-hoverxref rather than git+git://github.com/readthedocs/sphinx-hoverxref.git@master or something else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're aiming for another release next week ... fingers crossed :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. Ping us again in a week's time. :)

@kerimoyle
Copy link
Collaborator Author

Discussion here re getting release version with our changes: readthedocs/sphinx-hoverxref#71

@kerimoyle
Copy link
Collaborator Author

New release all done :) @agarny all good now?

requirements.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@agarny agarny left a comment

Choose a reason for hiding this comment

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

Just one minor thing.

@kerimoyle
Copy link
Collaborator Author

Right.... done ... @agarny ?

@kerimoyle kerimoyle requested a review from agarny May 26, 2020 22:29
Copy link
Contributor

@agarny agarny left a comment

Choose a reason for hiding this comment

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

Looks good to me, pending that CI works and passes everything once again. (I haven't checked, but just in case: @hsorby, any idea what is going on here?)

@kerimoyle
Copy link
Collaborator Author

kerimoyle commented May 26, 2020

There's no CI on this spec repo? And it's working fine, see rendered version here: https://cellml-specification-dev.readthedocs.io/en/i314_numref_hoverxref/reference/formal_and_informative/specC01_interpretation_of_imports.html (hover over the numbered links)

@agarny
Copy link
Contributor

agarny commented May 26, 2020

Oops, my bad, sorry! I was in libCellML mode... 🥴

@kerimoyle kerimoyle merged commit c5697f8 into cellml:cellml-2-drafting May 26, 2020
@kerimoyle kerimoyle deleted the i314_numref_hoverxref branch May 26, 2020 22:37
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.

It would be nice to have hoverxref working on all internal links...
2 participants