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 architecture documentation #599

Merged
merged 9 commits into from
Nov 7, 2023
Merged

Conversation

danielballan
Copy link
Member

Credit to @padraic-shafer for prompting this much needed addition

@padraic-shafer
Copy link
Contributor

Wow, I didn't even have to do any work and I get credit? 🤫

Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

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

Very nice!

I think that adding subheadings will help draw the reader's eye to key topics. Also they can be linked to in other docs & discussions.

docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Outdated Show resolved Hide resolved
docs/source/explanations/architecture.md Show resolved Hide resolved
@danielballan
Copy link
Member Author

danielballan commented Nov 3, 2023

Good edits. I'm stuck on getting any of the #adapter references to work; the build fails on:

Warning, treated as error:
/home/dallan/Repos/bnl/tiled/docs/source/explanations/architecture.md:40:'myst' cross-reference target not found: 'adapter' [myst.xref_missing]

@padraic-shafer
Copy link
Contributor

padraic-shafer commented Nov 3, 2023

For the links to 'H2' tags we might need to set myst_heading_anchors=2 in the sphinx build options. I haven't had to do this before, but it's referenced here.

@danielballan
Copy link
Member Author

I hope we can figure out how to make explicit targets work. I agree with this advice in the docs:

In general, it is discouraged to rely on implicit targets, since they are easy to break, if for example a document/heading is moved or renamed.

@padraic-shafer
Copy link
Contributor

I hope we can figure out how to make explicit targets work.

I'm trying it locally now. Will report back.

@danielballan
Copy link
Member Author

Ah, I thought that the target was an arbitrary string. I did not realize that the suffix -target was significant.

@padraic-shafer
Copy link
Contributor

padraic-shafer commented Nov 3, 2023

I think the '-target' suffix is arbitrary. However, it does remove the ambiguity of potential clash with the implicitly named targets, whose style I adopted here. That might be a bad naming choice on my part, but that's the reasoning. :)

@danielballan
Copy link
Member Author

Oh, you're right. Then I'm not sure what I was doing wrong in some local experiments earlier. Anyway, glad we have a working pattern now.

Copy link
Contributor

@dylanmcreynolds dylanmcreynolds left a comment

Choose a reason for hiding this comment

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

Would this be a good place to say (or link to) install client vs server via pip?

Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

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

Built locally; all ref links are working correctly.

@danielballan danielballan merged commit 8ae226b into bluesky:main Nov 7, 2023
8 checks passed
@danielballan danielballan deleted the dev-docs branch November 7, 2023 20:21
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.

3 participants