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

WIP: Add substitution definition code and tests #385

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

melissawm
Copy link
Collaborator

It took me long enough because I think I was trying to do too much. This is a first pass and definitely needs improvement but it does work. I need some feedback on whether this is the right approach and whether we should look at resolving external links too?

TODO:

  • Add tests for substitution definitions in ascii_expected (currently failing)
  • Implement substitution definitions for all section types in gen.py (including narrative and module docstrings)
  • Implement support for custom inline directives

@melissawm melissawm force-pushed the substitution-definition branch from d09bc21 to c1b4be0 Compare February 2, 2024 13:51
Copy link
Member

@Carreau Carreau left a comment

Choose a reason for hiding this comment

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

Let's try !

Thanks a lot.

dv = DVR(
qa,
known_refs,
local_refs=lr,
substitution_defs=substitution_defs,
Copy link
Member

Choose a reason for hiding this comment

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

👍

papyri/tree.py Show resolved Hide resolved
Copy link
Member

@Carreau Carreau left a comment

Choose a reason for hiding this comment

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

Let's try !

Thanks a lot.

self.children = [MImage(url=children[0].args, alt="")]
elif children[0].name == "replace":
self.children = [ReplaceNode(value=self.value, text=children[0].args)]
else:
Copy link
Member

Choose a reason for hiding this comment

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

Here is a quick search of what kind of subst we can find:

$ rg '^\.\. \|.+\| ' | cut -f3 -d'|'  | cut -f 1 -d':' |tr -d ' '| sort | uniq -c | sort -n
htb/2023-sept-responder/Responder/certs/responder.key: Permission denied (os error 13)
   1 baddata
   1 digraph
   1 directive
   1 https
   2 remplace
   2 types
   3 there'snodirectivehere
   4 hazard
   4 note
   6 date
   8 class
   8 d
  12 dir
  16 r
  24 raw
 298 image
 448 replace
2656 unicode

I think more generally a substition def should be .. |thename| directive-without-dot-dot:: content, so we might be able to not special case and just recurse on directive ?

Copy link
Collaborator Author

@melissawm melissawm Feb 5, 2024

Choose a reason for hiding this comment

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

That makes sense, I went with the docutils information but I guess there are other inline directives that are not docutils. I will give it a try. Marking this as a draft in the meantime oh I see you already did, thanks!

@Carreau
Copy link
Member

Carreau commented Feb 5, 2024

Ok, this is a bit more complicated that I thought, I've push a few commits to try to see how we can push this a bit further.

Comment on lines +87 to +88
def cbor(self, encoder):
assert False, self
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is currently failing on a table because of the |<table cell>| syntax in https://numpy.org/doc/stable/reference/generated/numpy.isscalar.html

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we can just add the part that fail on a table on hte ignore list. I don't think tree-sitter-rst plan to parse tables, and I'm wondering if having a .. table:: directive could be a good workaround, we can add it to sphinx as well, so that projects work on both papyri and sphinx at the same time.

@Carreau Carreau force-pushed the substitution-definition branch from c7a6ca7 to 8c28d3b Compare February 12, 2024 12:31
melissawm and others added 5 commits February 15, 2024 11:12
TODO:
- [ ] Add tests for substitution definitions in ascii_expected (currently failing)
- [ ] Implement substitution definitions for all section types in gen.py (including narrative and module docstrings)
- [ ] Implement support for custom inline directives
@Carreau Carreau force-pushed the substitution-definition branch from 8c28d3b to 265836e Compare February 15, 2024 10:12
This make SubstitutionDef (and Ref) unserialisable, to ensure things
fail at gen time.

We also mute a couple of warnings to make the log cleaner
@Carreau
Copy link
Member

Carreau commented Feb 15, 2024

This should be almost all good with the exception of numpy narrative that makes use of a global substitution using |version|, I guess we can either skip that for now or implement a global substitution.

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.

2 participants