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

Clean up the broken Class and Function links in docstrings #414

Merged
merged 18 commits into from
Apr 19, 2024

Conversation

yck011522
Copy link
Contributor

@yck011522 yck011522 commented Apr 12, 2024

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas_fab.robots.CollisionMesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@yck011522 yck011522 added the no changelog Disables changelog checker label Apr 12, 2024
@yck011522 yck011522 self-assigned this Apr 12, 2024
@@ -119,7 +119,7 @@ def __data__(self):


class AttachedCollisionMesh(Data):
"""Represents a collision mesh that is attached to a :class:`Robot`'s :class:`~compas_robots.model.Link`.
"""Represents a collision mesh that is attached to a :class:`Robot`'s :class:`compas_robots.model.Link`.
Copy link
Member

Choose a reason for hiding this comment

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

I actually like this with the ~ better, otherwise it is too verbose, don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea. But is it clear to people that compas_robots is a different package? At the moment if you pretend you are a user following that clickable link, you will reach a page that simply list the class Link. Somehow misleading that the Link class belongs to compas_fab.

I think if the API documentation page theme is slightly different between compas_fab and compas_robots, (maybe a different colour) then we can go with ~ for all the compas_robot links.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, it's useful to keep the full link in these cases, to highlight it's an external module. Altering the colors of fab would be a bit against the idea of having homogenous docs for all compas, so, let's stick with full link

@yck011522 yck011522 changed the title Yck011522/issue413 Clean up the broken Class and Function links in docstrings Apr 19, 2024
@yck011522
Copy link
Contributor Author

@gonzalocasas Fixed all your comments, we need to discuss only one thing. Whether to use the ~ for external links that points to compas_robots

@yck011522 yck011522 marked this pull request as ready for review April 19, 2024 04:41
Copy link
Member

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

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

Looking good! I just added a couple of suggestions for typos, otherwise, ready to go!

@@ -119,7 +119,7 @@ def __data__(self):


class AttachedCollisionMesh(Data):
"""Represents a collision mesh that is attached to a :class:`Robot`'s :class:`~compas_robots.model.Link`.
"""Represents a collision mesh that is attached to a :class:`Robot`'s :class:`compas_robots.model.Link`.
Copy link
Member

Choose a reason for hiding this comment

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

You're right, it's useful to keep the full link in these cases, to highlight it's an external module. Altering the colors of fab would be a bit against the idea of having homogenous docs for all compas, so, let's stick with full link

src/compas_fab/robots/robot.py Outdated Show resolved Hide resolved
src/compas_fab/robots/semantics.py Outdated Show resolved Hide resolved
src/compas_fab/robots/semantics.py Outdated Show resolved Hide resolved
@yck011522 yck011522 merged commit d49db17 into main Apr 19, 2024
13 checks passed
@yck011522 yck011522 deleted the yck011522/issue413 branch April 24, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Disables changelog checker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants