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

feat: add support for docs directive #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

crossk3
Copy link
Contributor

@crossk3 crossk3 commented Jun 29, 2023

Adds support for the !docs directive. This directive is valid for Workspaces, Software Systems, and Containers. However, kroki.io doesn't support !docs (obviously), so we need to be able to toggle whether we include these additional directives in the dump.

Unfortunately, because Models contain both Persons and SoftwareSystems, to fit the current implementation, Person.dump needs to accept with_docs argument and do nothing with it, while SoftwareSystem.dump acts on the argument. I don't like this, and am open to suggestions on a better implementation. I can see this getting worse when Group is supported, as it would also be an Element that needs to accept with_docs. There's also the additional complication of future !adrs support, etc.


This enables users to include docs=docs kwarg in any of the supported objects, run pystructurizr dump > workspace.dsl and then serve the dsl through a rendering engine of their choice that supports !docs. For example, I'm choosing to have the following file structure:

  • docs
    • doc.md
  • workspace.dsl

workspace.dsl includes directives such as !doc docs which when served via docker run -it --rm -v $(pwd):/var/model -p 8080:8080 ghcr.io/avisi-cloud/structurizr-site-generatr serve -w workspace.dsl imports the docs directory as appropriate.

@@ -74,7 +74,7 @@ def dump_relationships(self, dumper: Dumper) -> None:


class Person(Element):
def dump(self, dumper: Dumper) -> None:
def dump(self, dumper: Dumper, with_docs: bool = True) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dislike this, but because Workspace stores Persons and SoftwareSystems as List[Element], they both need to implement the same interface 😬

@nielsvanspauwen
Copy link
Owner

Thanks for the PR.

I'm not a huge fan of the !docs and !adrs directives, I find it strange that a tool that is essentially for diagrams-as-code is also used to generate a documentation site that then includes said diagrams.

Therefore, I'm not inclined to integrate this PR, especially as it complicates the code and makes the interface of some classes a bit strange, as you also indicate in your comments.

Since docs are just markdown files, why not use any other static site generator, and include the diagrams you generate with pystructurizr with regular ()[] markdown syntax?

@crossk3
Copy link
Contributor Author

crossk3 commented Jul 4, 2023

Thanks for the PR.

I'm not a huge fan of the !docs and !adrs directives, I find it strange that a tool that is essentially for diagrams-as-code is also used to generate a documentation site that then includes said diagrams.

I don't disagree with that notion, it does seem like an interesting choice that the DSL includes these directives, as opposed to embedding the diagrams within existing documentation.

Therefore, I'm not inclined to integrate this PR, especially as it complicates the code and makes the interface of some classes a bit strange, as you also indicate in your comments.

Unfortunately this is where I disagree, as long as the intent of this project is to provide Python bindings for the Structurizr DSL. If you only intend for this project to support the subset of the DSL that you use in your projects, then I've misunderstood and I'll take down my PR and use my fork. However, I think if this project is intending to be the Python equivalent for Structurizr DSL, implementing explicitly included DSL features should be supported, no matter our personal thoughts on those DSL design decisions.

I don't love what the changeset does for classes, but I think that's unavoidable based on the composition model of Structurizr's DSL.

Since docs are just markdown files, why not use any other static site generator, and include the diagrams you generate with pystructurizr with regular ()[] markdown syntax?

This is a reasonable implementation as well, but I'd prefer to stick within the existing Structurizr ecosystem by supporting official DSL features. It would also be good if existing ecosystem tools worked natively with pystructurizr, so someone using an arbitrary DSL rendering engine can use pystructurizr without question.


I don't intend to hijack your project and I appreciate the work you've put in to support the subset of DSL that the project currently supports. This is a feature of the DSL that people (including myself) use, so I think it's valuable to implement. I hope to be able to contribute groups as well in the future, which is a non-objectionable DSL feature unlike !docs and !adrs that also complicates the .dump() interface a little due to the composition model of the DSL.

Would love to continue to build on the great foundation you have here 🙂

@nielsvanspauwen
Copy link
Owner

Unfortunately this is where I disagree, as long as the intent of this project is to provide Python bindings for the Structurizr DSL. If you only intend for this project to support the subset of the DSL that you use in your projects, then I've misunderstood and I'll take down my PR and use my fork. However, I think if this project is intending to be the Python equivalent for Structurizr DSL, implementing explicitly included DSL features should be supported, no matter our personal thoughts on those DSL design decisions.

I think I'm somewhere in the middle:

  • It was never my intent to provide Python bindings for the Structurizr DSL, nor did I intend to replicate every feature that exists (and will exist) in Structurizr DSL.
  • At the same time, I don't want to restrict it to what I need for my projects, otherwise I would not have open sourced it.

If you look at the first statement in the README of this project, it states:

"PyStructurizr provides a Python DSL inspired by Structurizr, and is intended for generating C4 diagrams."

I think that captures my intent with this project well. I'm using Structurizr DSL and Kroki under the hood, because that seemed like a fast track to generating C4 diagrams. That doesn't mean PyStructurizr should be able to output every possible statement that is valid in the DSL.

For instance, the !constant directive and !include directive also don't make sense to me within PyStructurizr context.

This is a feature of the DSL that people (including myself) use, so I think it's valuable to implement. I hope to be able to contribute groups as well in the future, which is a non-objectionable DSL feature unlike !docs and !adrs that also complicates the .dump() interface a little due to the composition model of the DSL.

Adding groups support would be great indeed. Why would that complicate the .dump() interface though?

Would love to continue to build on the great foundation you have here 🙂

I would like that as well!

Can we find a less obtrusive way of adding the !docs and !adrs directives? For instance, what about generalizing it to a .add_directive(txt: str) function on the Model class. Combined with an include_directives state variable on the Dumper class, so individual Element subclasses don't need to be aware of whether they are dumping in dev or build context?

@crossk3
Copy link
Contributor Author

crossk3 commented Jul 14, 2023

Sorry for delay, had a short vacation 🙂


For instance, what about generalizing it to a .add_directive(txt: str) function on the Model class. Combined with an include_directives state variable on the Dumper class, so individual Element subclasses don't need to be aware of whether they are dumping in dev or build context?

This feels like a much better implementation. I'll work on implementing this.

I also agree that there's some "interesting" choices made in the DSL, including the !docs directive. I'd definitely advocate more for "soft feature parity" than yourself perhaps, but I also completely understand the trepidation. Keeping the implementation clean will go a long way on reducing the mess created by odd things such as !docs.

@simonbrowndotje
Copy link

simonbrowndotje commented Jul 20, 2023

I don't disagree with that notion, it does seem like an interesting choice that the DSL includes these directives, as opposed to embedding the diagrams within existing documentation.

I suspect that most users of Structurizr are embedding diagrams into existing documentation ... whether that's Markdown/AsciiDoc (via Hugo, Jekyll, etc) or Confluence/SharePoint/etc. The Structurizr cloud service/on-premises installation/Lite provides a simple documentation generator for smaller teams who may not have anything already, which is why the DSL supports !docs and !adrs. Those embedded diagrams are also interactive (demo), which is a feature missing when embedding PNGs.

I'll take down my PR and use my fork

@crossk3 ... the existing Structurizr for Python library might be worth taking a look at. It was recently archived (because the author uses the Structurizr DSL instead), but might provide some inspiration for adding docs support (by which I mean the class structure ... it also doesn't seem to support docs).

@crossk3 crossk3 force-pushed the kcross/add-docs-support branch from 7a341c7 to 07e7ffe Compare August 3, 2023 21:20
@crossk3
Copy link
Contributor Author

crossk3 commented Aug 3, 2023

@nielsvanspauwen Sorry for the extreme delay, I see you beat me to the punch on groups! Thanks for that 🙏

I've updated the implementation of directives to be less intrusive on the object models, and now the decision to include the directives is on Dumper where it belongs 🙂

Hopefully the new implementation is less objectionable, lol. Although this PR relies on the "fix tests" PR, so those changes are on this branch as well.

@crossk3 crossk3 force-pushed the kcross/add-docs-support branch 2 times, most recently from 488d2bd to 3c613bb Compare August 3, 2023 21:27
@crossk3 crossk3 force-pushed the kcross/add-docs-support branch from 3c613bb to 87d80db Compare August 4, 2023 21:29
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