Skip to content

Conversation

@hamelsmu
Copy link
Collaborator

Sorry this took so long, I was trying to figure out how to deal with the table rendering, as escaping things in tabulate can be quite hairy! To complicate things a bit further, when you define something with string "foo | bar" that annotation element is of type str, instead of type expression so right now made a tradeoff that we can talk through.

I also need to ask about how you want to go about tests on this. I see there are some tests already for interlinks, but it would be good to get a walk-through from you how you envision that working, since there is already some code there.

@hamelsmu
Copy link
Collaborator Author

hamelsmu commented May 23, 2023

Oh yes I think it would be helpful to talk through the failing tests live. I couldn't quite figure out why there was a problem because the site rendered just fine throughout -> nevermind misread the output I just fixed it

@machow
Copy link
Owner

machow commented May 25, 2023

@hamelsmu sorry for creating a merge conflict 😓. griffe now exposes Expression and Name in the griffe.expressions submodule. I merged main back into this branch, but definitely feel free to force push up what you / reset the merge 👼

@hamelsmu
Copy link
Collaborator Author

@machow These pre-commit hooks are not smooth at all and really slow down development. I'm not sure it is the right approach and is very heavy handed. When I'm iterating on the pre-commit hooks themselves you have to do something like git commit --no-verify -m'fix tests' because it relies on the commited files even after clearing the cache, and an upstream fix like black can interfere with a downstream test like flake8. It's a bit frustrating IMO, spent about an hour arm wrestling it.

@has2k1
Copy link
Contributor

has2k1 commented Aug 22, 2023

These pre-commit hooks are not smooth at all and really slow down development ...
... and an upstream fix like black can interfere with a downstream test like flake8

I have had great experience with ruff. It effectively replaces all of flake8, omitting the rules that clash with black. It is fast and it can fix many violations.

@has2k1
Copy link
Contributor

has2k1 commented Aug 22, 2023

... I was trying to figure out how to deal with the table rendering

I think rendering the parameter section as a table is convenient but it is not the best fit. As the parameter section contains definitions, it should map onto definition lists. In html, they become description lists.

A minor inconvenience with this it that definitions lists are not part of any markdown flavor but are a pandoc extension and they must be enabled explicitly in the config file. i.e.

from: markdown+definition_lists

And as html, they definitely require more css styling attention than tables but I think there is room for some good styling options.

@machow
Copy link
Owner

machow commented Aug 22, 2023

I've opened an issue specifically to track @has2k1 's point of rendering tables in a different style (description lists). This has come up a fair amount (e.g. from the shiny team), and is a bit different from the issue mentioned above (escaping inside a table; assuming you want it in table format).

I want to streamline interlinks before conf, so can circle back and incorporate this PR into w/e is needed there!

#247

@machow machow changed the base branch from main to builder-all-dynamic August 31, 2023 18:53
@machow machow changed the base branch from builder-all-dynamic to main August 31, 2023 18:53
@machow
Copy link
Owner

machow commented Aug 31, 2023

addresses #135

@machow
Copy link
Owner

machow commented Aug 31, 2023

I think something weird happened with black formatting in this PR. AFAICT, baac169 applied black, but because .flake8 was missing the header, it reformatted everything to be <= 80 characters.

@machow
Copy link
Owner

machow commented Aug 31, 2023

I tweaked a bit, and tested on our docs and a bit on the ibis docs, and it seems to be working pretty well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants