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

Syntax documentation + new /docs directory #111

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

formatc1702
Copy link
Collaborator

@formatc1702 formatc1702 commented Jul 20, 2020

Closes #107.

See current status in rendered form here.

@formatc1702 formatc1702 added the WIP Work in progress label Jul 20, 2020
@formatc1702 formatc1702 added this to the v0.2 milestone Jul 20, 2020
@formatc1702 formatc1702 self-assigned this Jul 20, 2020
@formatc1702
Copy link
Collaborator Author

formatc1702 commented Jul 20, 2020

Obviously this is a work in progress, so things are missing. Please add your comments keeping this in mind.

I thought about doing this via docstrings, but I think a separate documentation, for non-programmers, is a valuable addition.

I want to keep it as minimalistic as possible, to be helpful as a quick reference guide, and easy to use alongside the existing tutorial. I'm trying to avoid long paragraphs of text.

@kvid
Copy link
Collaborator

kvid commented Jul 20, 2020

@slightlynybbled This could also be a place for the documentation you are missing, as I described in #95 (comment). What do you think?

@formatc1702
Copy link
Collaborator Author

@slightlynybbled This could also be a place for the documentation you are missing, as I described in #95 (comment). What do you think?

That's my intention with the ## Multiline strings section 😉

@slightlynybbled
Copy link
Contributor

I see that you are generating a markdown-based documentation. That is a great start and would certainly be a place to put the 'gotchas' that documentation typically contains.

What is 1yr vision for documentation? readme.md? Sphinx? MkDocs?

@formatc1702
Copy link
Collaborator Author

What is 1yr vision for documentation? readme.md? Sphinx? MkDocs?

I will have to think about it and read up on these systems before I choose. Perhaps we can open a new issue to discuss this?

@formatc1702 formatc1702 force-pushed the feature/syntax-description branch 4 times, most recently from f5b2f91 to 93a00cf Compare July 20, 2020 18:27
@aakatz3
Copy link
Contributor

aakatz3 commented Jul 21, 2020

What are your thoughts on including comprehensive documentation (and possibly snippets from the other markdown files) in the repo wiki, so it's easily browseable by first-time users? The markdown files look great, but it would be nice to either have a docs folder, a github-pages site, or a wiki with them for accessibility.

@formatc1702
Copy link
Collaborator Author

  • Short term: Add syntax.md and link to it from the main readme.md
  • Long term: Proper documentation as @slightlynybbled suggested, system is TBD
  • Honestly, I wouldn't want to start a wiki; I'd rather have a dedicated (and mostly autogenerated?) GH Pages repo.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Jul 24, 2020

While there are a couple of open to-do's, the syntax description has reached a stage where I'm happy with the information that's included. It is rather dense, since it's not meant as a gentle intro, but as a reference guide.

I believe most of the WireViz syntax itself is described without any glaring omissions. Please have a read and provide feedback if

  • a feature/attribute/behavior is missing
  • something is described in an utterly incomprehensible, ambiguous or contradictory way
  • you think it could be stylistically improved, language wise

@slightlynybbled @kvid @Tyler-Ward @aakatz3

@Tyler-Ward
Copy link
Contributor

The style and layout looks good to me, should work well as a reference for people writing input files.

There are a few references I spotted to features for 0.3 e.g #50 and #48, these are marked as todo in the code so I assume you are already aware of them.

@formatc1702
Copy link
Collaborator Author

There are a few references I spotted to features for 0.3 e.g #50 and #48, these are marked as todo in the code so I assume you are already aware of them.

I've removed references to future features; it's probably best to add them to the documentation once they are actually implemented.

@formatc1702 formatc1702 removed the WIP Work in progress label Jul 27, 2020
@formatc1702 formatc1702 marked this pull request as ready for review July 27, 2020 19:40
@formatc1702 formatc1702 mentioned this pull request Aug 1, 2020
@formatc1702 formatc1702 force-pushed the feature/syntax-description branch 2 times, most recently from b80ac0e to b0ecbae Compare August 1, 2020 20:28
@formatc1702 formatc1702 changed the title Add WireViz syntax documentation Syntax documentation + new /docs directory Aug 1, 2020
docs/buildscript.md Outdated Show resolved Hide resolved
docs/buildscript.md Outdated Show resolved Hide resolved
## Connector attributes

```yaml
<str> : # unique connector designator/name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the lexical item designator defined in more detail? Are there some limitations?

  • Legal characters (including space).
  • How can any illegal characters be quoted?
  • Any limits on the number of characters?

Copy link
Collaborator Author

@formatc1702 formatc1702 Aug 13, 2020

Choose a reason for hiding this comment

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

This is probably a very deep rabbit hole, with all kinds of exceptions:

  • numbers (without letters) are accepted, but need to go in quotes so they are parsed as strings
  • spaces are OK
  • some special YAML characters (-, : and others) can be used by putting the string in quotes
  • some characters will be accepted, but
    • might not show up in the graphical output
    • might mess with the BOM export
  • some obscure cases might throw errors

Honestly for now I would leave the explanation as-is, and let the user figure it out.
I do agree that it merits a more detailed description, but I would leave that for a later date.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As this connector designator (and similar for cables) currently is used directly to name the Graphviz node, it must be case insensitive unique within the union of connectors and cables in the harness to avoid reference mix-ups similar to those discovered in #160.

Copy link
Collaborator Author

@formatc1702 formatc1702 Oct 14, 2020

Choose a reason for hiding this comment

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

I would like to leave it as-is now, hopefully fix #160 soon and then add "case-sensitive" to the syntax description.

Otherwise it's a confusing mix... "It's case-sensitive for Python but still please treat it as case-insensitive to avoid GraphViz issues 😅 "

docs/syntax.md Outdated Show resolved Hide resolved
Comment on lines +39 to +40
type: <str>
subtype: <str>
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Same comment as above: Are there some limitations about a string like this?
  • If a designator can be any string, then these descriptions can be combined.
  • If the only limitations are what can be specified in YAML, then a link to a description, might be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keep simple for now, so this won't be changed at the moment.
Since it's a string inside a YAML file, adherence to the YAML spec is implicit. I wouldn't add links to what YAML treats as an int or float here either...

@formatc1702 formatc1702 force-pushed the feature/syntax-description branch 2 times, most recently from 14cbe7a to 663833b Compare August 13, 2020 16:06
docs/syntax.md Outdated Show resolved Hide resolved
docs/syntax.md Outdated
caption: <str> # text to display below the image
width: <int> # range: 1~65535; unit: ???
height: <int> # range: 1~65535; unit: ???
fixedsize: <bool> # ???
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest removing this here as it is now explained in the advanced usage file (see link above).

docs/syntax.md Outdated
Comment on lines 324 to 330
scale: <str> # scaling behavior; possible values:
# false does not scale the image
# true scale image proportionally
# to fit within given max. width and height
# width scale image proportionally to given width
# height scale image proportionally to given height
# both resize to given width and height
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest removing this here as it is now explained in the advanced usage file (see link above).

docs/syntax.md Outdated
Comment on lines 336 to 337
Connectors accept multiline strings in the `type`, `subtype` and `notes` attributes.
Cables accept multiline strings in the `type` and `notes` attributes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Include also these for both connectors and cables: manufacturer, mpn, image.caption

As the list is almost identical for both connectors and cables, I suggest combining their descriptions like this:

Connectors and cables accept multiline strings in these attributes: manufacturer, mpn, notes, image.caption, type and subtype (the latter doesn't exist for cables).

Alternatively, we could consider using something like <mlstr> as type where a multiline string is accepted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, we could consider using something like <mlstr> as type where a multiline string is accepted.

After adding semanticly named type aliases in PR #163, I now suggest we do something similar here. If PR #163 is accepted, I suggest we use the same type names here as in DataClasses.py. See also #163 (comment) about how PR #164 might affect this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alternatively, we could consider using something like <mlstr> as type where a multiline string is accepted.

After adding semanticly named type aliases in PR #163, I now suggest we do something similar here. If PR #163 is accepted, I suggest we use the same type names here as in DataClasses.py. See also #163 (comment) about how PR #164 might affect this.

Let's think about this in the future, once those PRs are actually merged :) For now, a list of multiline-supporting attributes is enough.

docs/syntax.md Outdated

## Inheritance

Add link to YAML spec.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this link to YAML explanations a good candidate? https://learnxinyminutes.com/docs/yaml/
It seems to cover what the users might need (and a bit more), but it is not limited to inheritance, so the section header might be changed to General YAML tutorial or something like that. If you prefer a link to inheritance only, then this archived article might be a very simple candidate: https://web.archive.org/web/20130213112648/http://blog.101ideas.cz/posts/dry-your-yaml-files.html

A related issue is the main section templates as used in demo02.yml, ex05.yml, and ex06.yml. This main section is not explained here probably because it is ignored by the WireViz parser. The templates name is just a dummy placeholder for defining a list of templates without actually including them in the harness.

I suggest documenting the templates main section as part of the syntax (supporting the usage in example files) even though the user can actually name it anything that is not interpreted as something else.

docs/syntax.md Outdated
Comment on lines 50 to 52
# pinout information
# at least one of the following must be specified
# if more than one is specified, list lenghts must match pincount, and each other
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is actually allowed to set pincount > len(pins) and it does make sense, e.g. in cases like those I suggested in #138 where the remaining pins appear as hidden.

## Connector attributes

```yaml
<str> : # unique connector designator/name
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this connector designator (and similar for cables) currently is used directly to name the Graphviz node, it must be case insensitive unique within the union of connectors and cables in the harness to avoid reference mix-ups similar to those discovered in #160.

docs/README.md Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/syntax.md Outdated
Comment on lines 336 to 337
Connectors accept multiline strings in the `type`, `subtype` and `notes` attributes.
Cables accept multiline strings in the `type` and `notes` attributes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, we could consider using something like <mlstr> as type where a multiline string is accepted.

After adding semanticly named type aliases in PR #163, I now suggest we do something similar here. If PR #163 is accepted, I suggest we use the same type names here as in DataClasses.py. See also #163 (comment) about how PR #164 might affect this.

docs/syntax.md Outdated

## Inheritance

Add link to YAML spec.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also mention here how templates can be reused between harnesses by defining them in a separate YAML file that can be included with the --prepend-file command line option?

docs/README.md Show resolved Hide resolved
docs/buildscript.md Show resolved Hide resolved
docs/syntax.md Outdated
Comment on lines 218 to 219
Connectors (both regular, and auto-generated), cables, and wires of a bundle are automatically added to the BOM,
unless the `ignore_in_bom` attribute is set to `true`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

ignore_in_bom is not yet implemented. See #50 and #115.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the ignore_in_bom feature is wanted for 0.2 it can be fairly easily merged in on its own as it is the first commit in the branch for #115 (716cc06)

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

Suggesting additions to Contribution Guidelines

docs/CONTRIBUTING.md Show resolved Hide resolved
docs/CONTRIBUTING.md Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

I've included a lot of your suggestions, and commented on the ones where I think it's worth leaving as-is for now and expanding in the future, so as not to overthink it in this still early stage :) I hope you agree on these points, looking forward to finishing up and merging the current version of the docs.

@formatc1702
Copy link
Collaborator Author

If there are no major disagreements, @kvid please give green light to squash and merge :)

Otherwise let me know, I've hopefully addressed all your comments. Thanks!

@kvid
Copy link
Collaborator

kvid commented Oct 14, 2020

If there are no major disagreements, @kvid please give green light to squash and merge :)

Otherwise let me know, I've hopefully addressed all your comments. Thanks!

I'm sorry, but I don't have a PC where I am right now. I only have a phone with a small screen. I have to wait until later today before I'm able to read your latest effort properly. However, I do know about a few issues:

  • Have you updated the doc with the latest command line option changes of both wireviz and build_examples? Run both of them with --help to see the new options.
  • I have made a list at home of PRs that are missing from the CHANGELOG.md, and at least one bug in the existing references.

@kvid
Copy link
Collaborator

kvid commented Oct 14, 2020

  • I have made a list at home of PRs that are missing from the CHANGELOG.md, and at least one bug in the existing references.

Please browse quickly through the references to check that I didn't misinterpreted any of them.

Missing change log entries (feel free to join minor or related changes together):

Extra reference to existing items:

Bad existing reference:

TODO:

@kvid
Copy link
Collaborator

kvid commented Oct 14, 2020

If there are no major disagreements, @kvid please give green light to squash and merge :)

Otherwise let me know, I've hopefully addressed all your comments. Thanks!

I can't find any reaction (comment or code change) related to my #111 (comment) - did you consider this one?

@formatc1702
Copy link
Collaborator Author

Wow, that's an exhaustive list, amazing! When I started the changelog, I thought I'd just include the most relevant changes, but perhaps it's good practice to include the complete list. Thanks. I will do this tomorrow.

I can't find any reaction (comment or code change) related to my #111 (comment) - did you consider this one?

I've updated buildscript.md to reflect the changes suggested in your linked comment, and updated README.md to show wireviz.py's CLI options.

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

Be aware that #111 (comment) has been edited since your last message in this PR.

docs/README.md Outdated Show resolved Hide resolved
docs/buildscript.md Show resolved Hide resolved
Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

I overlooked this one yesterday.

docs/buildscript.md Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

I've added all the issue/PR links you provided. Hopefully we can close this now, ideally I'd like to package up v0.2 today since I have some time.

docs/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@kvid kvid 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 for accepting almost all of my suggestions. And thank you for the effort creating this nice documentation. Too many projects neglect documentation in the early stages of a project.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Oct 16, 2020

Thank you for accepting almost all of my suggestions. And thank you for the effort creating this nice documentation. Too many projects neglect documentation in the early stages of a project.

  1. It's really annoying having to hand-pick and document every little change and keep things tidy...
  2. But imagine having to do that 1, 2, 6 or 12 months from now! 😅
  3. Thanks for taking care of 99% of the work here.

Please have a look at #178 in case I'm forgetting something... I'm trying to make a foolproof checklist, which I can then reuse on future releases, so feel free to add your thoughts.

I'm excited to finish this PR but will wait until your final 👍 .
Edit: I missed the approval message before your comment. Thanks!

- Create `docs/` directory
- Add syntax description (`syntax.md`)
- Track changes since first release (`CHANGELOG.md`)
- Expand contribution guidelines (`CONTRIBUTING.md`)
- Improve main readme (`README.md`)
- Add documentation for build script (`buildscript.md`)
@formatc1702 formatc1702 merged commit c9cc94d into dev Oct 16, 2020
@formatc1702 formatc1702 deleted the feature/syntax-description branch October 16, 2020 20:44
@kvid
Copy link
Collaborator

kvid commented Oct 16, 2020

Thank you for accepting almost all of my suggestions. And thank you for the effort creating this nice documentation. Too many projects neglect documentation in the early stages of a project.

  1. It's really annoying having to hand-pick and document every little change and keep things tidy...
  2. But imagine having to do that 1, 2, 6 or 12 months from now! 😅

I suggest adding a minor entry to the change log immediately after merging any PR. It should be much easier when it is fresh in mind, instead of searching all PRs just before a release. 😃

BTW, you just merged my PR #177 in case that fits into one of the change log items - perhaps the item about "Improve HTML output".

  1. Thanks for taking care of 99% of the work here.

Not at all. A lot of minor suggestions only. You have put everything together.

Please have a look at #178 in case I'm forgetting something... I'm trying to make a foolproof checklist, which I can then reuse on future releases, so feel free to add your thoughts.

I'm excited to finish this PR but will wait until your final 👍 .

@formatc1702
Copy link
Collaborator Author

I suggest adding a minor entry to the change log immediately after merging any PR. It should be much easier when it is fresh in mind, instead of searching all PRs just before a release. 😃

BTW, you just merged my PR #177 in case that fits into one of the change log items - perhaps the item about "Improve HTML output".

🤦 Yes, this should become standard practice. Time for a quick force-push to dev... 🤫 You didn't see anything!

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.

5 participants