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

Add documentation for string formats; fix at-identifier format #491

Merged
merged 14 commits into from
Dec 17, 2024

Conversation

zzstoatzz
Copy link
Contributor

@zzstoatzz zzstoatzz commented Dec 13, 2024

closes #478

this PR adds:

@zzstoatzz zzstoatzz changed the title try to add docs [wip] add string format docs Dec 13, 2024
@zzstoatzz zzstoatzz force-pushed the add-string-format-docs branch 4 times, most recently from 2359e6e to 4e97cc8 Compare December 13, 2024 01:41
@zzstoatzz zzstoatzz changed the title [wip] add string format docs add string format docs Dec 13, 2024
@zzstoatzz zzstoatzz marked this pull request as ready for review December 13, 2024 01:49
@zzstoatzz zzstoatzz force-pushed the add-string-format-docs branch 3 times, most recently from b27b890 to d1eaf42 Compare December 14, 2024 03:05
Copy link
Owner

@MarshalX MarshalX left a comment

Choose a reason for hiding this comment

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

thank you! could you pls investigate how to fix this rendering (screnshot)? happened after merging PR with string formats

image

link: https://atproto--491.org.readthedocs.build/en/491/atproto/atproto_client.models.app.bsky.graph.get_follows.html#atproto_client.models.app.bsky.graph.get_follows.Params.actor

packages/atproto_client/models/string_formats.py Outdated Show resolved Hide resolved
packages/atproto_client/models/string_formats.py Outdated Show resolved Hide resolved
remove redundant type info

dont interfere w docstring
try again

jeez

try to fix spacing
no markdown i guess
@zzstoatzz zzstoatzz force-pushed the add-string-format-docs branch from d8f75c5 to b63171b Compare December 16, 2024 00:09
clean up a couple things
@zzstoatzz zzstoatzz force-pushed the add-string-format-docs branch 2 times, most recently from 4c5e52c to 967a9c6 Compare December 16, 2024 01:44
@zzstoatzz zzstoatzz force-pushed the add-string-format-docs branch from 1fd79c3 to 7e97961 Compare December 16, 2024 06:29
@zzstoatzz
Copy link
Contributor Author

@MarshalX alright I have

  • updated the docstrings to follow google convention
  • added a string_formats.md (let me know what you think!)
  • explored the weird <locals>.wrapper rendering a bit more

on the last point, the best way I could immediately find to deal with this was to switch to using @wraps(validate_fn) in only_validate_if_strict, since that properly handles carrying metadata without manually patching __doc__ or __name__. However, as a consequence this forces me to update each validator signature to at least (v: str, _: ValidationInfo). With this in place, we see a more reasonable rendering

image

while the address present is midly annoying to me, showing the function name makes some sense to me as the constraint really is arbitrary logic that may exist in that function. let me know if you see a more reasonable path forward on this!

@zzstoatzz zzstoatzz force-pushed the add-string-format-docs branch from 7e97961 to 0f6168e Compare December 16, 2024 06:49
@MarshalX
Copy link
Owner

string_formats.md is amazing! well done!

i dig into the problem around rendering of validators. the answer is simple: the lib which we use to autodoc pydantic models simply does not support it.

so i've added a hacky quick win to be able to merge it:
image

@zzstoatzz
Copy link
Contributor Author

zzstoatzz commented Dec 16, 2024

great @MarshalX ! sounds good

@MarshalX MarshalX changed the title add string format docs Add documentation for string formats; fix at-identifier format Dec 16, 2024
@MarshalX
Copy link
Owner

@zzstoatzz i fixed some stuff; pls check and confirm is all good

Copy link
Contributor Author

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

nice! lgtm

@MarshalX MarshalX merged commit c2a51c7 into MarshalX:main Dec 17, 2024
23 checks passed
@MarshalX
Copy link
Owner

thank you!

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.

add documentation for string formats
2 participants