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

MAINT: review of docs, minor maintenance, and bug fix #105

Merged
merged 4 commits into from
Jul 15, 2024
Merged

Conversation

mmcky
Copy link
Member

@mmcky mmcky commented Jul 2, 2024

This PR

  • reviews docs
  • reviews base level files for configuration and tidies up (enables tox.ini and removes TODO as captured in issues)
  • fixes resolve_any_xref warning issued by myst parser (fixes BUG: sphinx proof is generating warnings #97)

@mmcky mmcky changed the title MAINT: review of docs and minor maintenance MAINT: review of docs, minor maintenance, and bug fix Jul 2, 2024
@mmcky mmcky requested a review from agoose77 July 2, 2024 03:50
@mmcky
Copy link
Member Author

mmcky commented Jul 2, 2024

@agoose77 from my reading of myst-parser and myst-nb code I think the fix for resolve_any_xref is the right way to go but let me know if it should refer on to resolve_xref instead.

@agoose77
Copy link
Contributor

agoose77 commented Jul 2, 2024

I think the proper fix is to replace what myst-parser is doing here, and rename resolve_xref to resolve_xref_any (assuming we always pull in a version of myst-parser that supports this, which I think is true).

@mmcky
Copy link
Member Author

mmcky commented Jul 2, 2024

I think the proper fix is to replace what myst-parser is doing here, and rename resolve_xref to resolve_xref_any (assuming we always pull in a version of myst-parser that supports this, which I think is true).

Thanks @agoose77 I can't find any instances of resolve_xref_any searching for repo:executablebooks/MyST-Parser resolve_xref_any so maybe I am missing something.

Reading https://www.sphinx-doc.org/en/master/extdev/domainapi.html#sphinx.domains.Domain.resolve_any_xref it seems that it is an old method that is only used when a request is coming from an any role (with unknown type). I saw that myst-nb just returns an empty list which is the approach I have taken here.

@agoose77
Copy link
Contributor

agoose77 commented Jul 3, 2024

@mmcky ah, my mistake -- it's resolve_any_xref!

@mmcky
Copy link
Member Author

mmcky commented Jul 3, 2024

No worries @agoose77 I can't find any methods for resolve_any_xref either in myst-parser. Just some logic around checking for it. Am I missing something?

https://github.com/search?q=repo%3Aexecutablebooks%2FMyST-Parser%20resolve_any_xref&type=code

@agoose77
Copy link
Contributor

agoose77 commented Jul 3, 2024

Sorry @mmcky - I'm between things today. Here's a hard link that should clarify whatever the correct thing is!

https://github.com/executablebooks/MyST-Parser/blob/master/myst_parser%2Fsphinx_ext%2Fmyst_refs.py#L245

@mmcky
Copy link
Member Author

mmcky commented Jul 4, 2024

thanks @agoose77. I have implemented method forwarding similarly to myst-parser.

@mmcky
Copy link
Member Author

mmcky commented Jul 10, 2024

@agoose77 we would love to have the updated styling available for one of our projects. Would it be OK if I merge this and #106

@agoose77
Copy link
Contributor

@mmcky thanks for being so patient - been in transit for scipy. Will take a look tomorrow, but honestly merge it if you're happy. It looked reasonable to me last I checked in.

@mmcky
Copy link
Member Author

mmcky commented Jul 15, 2024

Thanks @agoose77 I will merge this. It is passing tests so I'm happy with it but we can loop around and make improvements if required.

Look forward to hearing about SciPy.

@mmcky mmcky merged commit 6f35331 into main Jul 15, 2024
7 checks passed
@mmcky mmcky deleted the maint-02jul branch July 15, 2024 22:43
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.

BUG: sphinx proof is generating warnings
2 participants