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 missing documentation for GCODE analysis and other small fixes #2540

Merged
merged 1 commit into from Apr 4, 2018

Conversation

dforsi
Copy link
Contributor

@dforsi dforsi commented Apr 2, 2018

Adds documentation for dimensions and printingArea; fixes length
and volume; adds two internal references.
Fixes #2463.

  • Your changes are not possible to do through a plugin and relevant
    to a large audience (ideally all users of OctoPrint)
  • If your changes are large or otherwise disruptive: You have
    made sure your changes don't interfere with current development by
    talking it through with the maintainers, e.g. through a
    Brainstorming ticket
  • Your PR targets OctoPrint's devel branch, or maintenance if it's
    a bug fix for an issue present in the current stable version (no PRs
    against master or anything else please)
  • Your PR was opened from a custom branch on your repository
    (no PRs from your version of master, maintenance or devel please),
    e.g. dev/my_new_feature or fix/my_bugfix
  • Your PR only contains relevant changes: no unrelated files,
    no dead code, ideally only one commit - rebase and squash your PR
    if necessary!
  • Your changes follow the existing coding style
  • If your changes include style sheets: You have modified the
    .less source files, not the .css files (those are generated with
    lessc)
  • You have tested your changes (please state how!) - ideally you
    have added unit tests
  • You have run the existing unit tests against your changes and
    nothing broke
  • You have added yourself to the AUTHORS.md file :)

What does this PR do and why is it necessary?

It adds some missing information.

How was it tested? How can it be tested by the reviewer?

make -C docs html

Any background context you want to provide?

What are the relevant tickets if any?

#2463

Screenshots (if appropriate)

image

Further notes

Now the structure of this JSON object is too complex to be rendered in a simple flat table. I'm open to ideas.
I don't think that it is easy to understand that the table above can describe a fragment like this (I may have made a mistake):

filament: {tool0: {length: 1407.434510000002, volume: 8.978582902758397}}

@@ -384,14 +389,58 @@ GCODE analysis information
- 0..1
- Object
- The estimated usage of filament
* - ``filament.length``
* - ``tool{n}.filament.length``
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this rather be filament.tool{n}.length? Same with filament.tool{n}.volume below.

@foosel foosel added the needs some work There are some things to do before this PR can be merged label Apr 3, 2018
@foosel
Copy link
Member

foosel commented Apr 3, 2018

I think a flat table should still suffice, as long as the above issue is fixed.

I would expect people to also take a look at live data which should clarify things entirely. Or we could just include an example?

Adds documentation for `dimensions` and `printingArea`; fixes `length`
and `volume`; adds two internal references.
Fixes OctoPrint#2463.
@dforsi
Copy link
Contributor Author

dforsi commented Apr 3, 2018

You were right, I fixed the two filament.tool{n}... lines and forced a push.

Including an example would be useful, but only if you can add them in a form that can be tested, such as using unit tests as examples (maybe pulling the code in the .rst file with literalinclude), or deriving the json output from running actual code (maybe with doctest), so that only the tables are to be maintained manually, but I never used those directives. In some cases it would be better for the reader to see only some fields of a big object, however appending to each chapter an output with all possible fields would be easier to maintain, if you can derive it form the code.

@foosel foosel merged commit 561bf83 into OctoPrint:maintenance Apr 4, 2018
@foosel foosel added this to the 1.3.9 milestone Jun 18, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs some work There are some things to do before this PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants