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

update descriptions in Lexicons #2110

Merged
merged 4 commits into from
Feb 12, 2024
Merged

update descriptions in Lexicons #2110

merged 4 commits into from
Feb 12, 2024

Conversation

bnewbold
Copy link
Collaborator

@bnewbold bnewbold commented Jan 30, 2024

Probably need to do a codegen to update comments etc, but i'm not sure if that might cause merge conflicts with large feature branches.

Annotated many ambiguous endpoints as requiring auth (or not), but did not annotate all. Likewise annotated some as being implemented by specific network services, but didn't do this for every endpoint. This was taking a long time so I mostly just included stuff I was confident about, but might be ambiguous or confusing to devs.

Motivation here is to improve auto-generated docs.

Emily tagged for copy-editing and style, Devin for factual correctness.

Copy link
Collaborator

@emilyliu7321 emilyliu7321 left a comment

Choose a reason for hiding this comment

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

all minor copy tweaks

lexicons/app/bsky/embed/images.json Outdated Show resolved Hide resolved
lexicons/app/bsky/embed/images.json Outdated Show resolved Hide resolved
lexicons/app/bsky/embed/images.json Outdated Show resolved Hide resolved
lexicons/app/bsky/embed/images.json Outdated Show resolved Hide resolved
lexicons/app/bsky/embed/recordWithMedia.json Outdated Show resolved Hide resolved
lexicons/app/bsky/feed/describeFeedGenerator.json Outdated Show resolved Hide resolved
lexicons/app/bsky/feed/getFeed.json Outdated Show resolved Hide resolved
lexicons/app/bsky/feed/getListFeed.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@dholms dholms left a comment

Choose a reason for hiding this comment

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

this is great, thanks for putting these together 🙌

@bnewbold
Copy link
Collaborator Author

bnewbold commented Feb 3, 2024

@dholms when I went to rebase, codegen, and merge this, I got some scary diffs like:

@@ -167,39 +167,39 @@ export class Server {
 
 export class ComNS {
   _server: Server
-  atproto: ComAtprotoNS
+  atproto: AtprotoNS

is there some refactor in-flight?

This PR is docs-only, so i'm open to merging as-is; or if somebody else knows how to wrangle the codegen. It would be great to get this in by early next week, but also not a huge deal.

@mackuba
Copy link

mackuba commented Feb 6, 2024

See also: #1713

@bnewbold
Copy link
Collaborator Author

bnewbold commented Feb 6, 2024

Ooookey doke, figure out I was just missing make build and then codegen looks right. It's a doozy.

Also did a rebase before codegen.

closes: #1713

@dholms
Copy link
Collaborator

dholms commented Feb 8, 2024

Ah yup sorry! We updated the lexgen cli & you probably out an out of date build

@bnewbold bnewbold merged commit 9579bec into main Feb 12, 2024
10 checks passed
@bnewbold bnewbold deleted the bnewbold/lex-desc branch February 12, 2024 00:06
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.

4 participants