-
-
Notifications
You must be signed in to change notification settings - Fork 60
Documentation Updates as per #697 #698
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
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this update; a couple of notes inline, in addition to the pre-commit issue that CI has flagged.
changes/697.doc.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change note isn't the same thing as a commit message - it's the content that will be put into the final release notes. That means it needs to be in a voice of "Rubicon now has long awaited shiny new feature!" or "Rubicon no longer breaks on Tuesday afternoons".
In this case, it probably doesn't even rate a release note - although this is a change to documentation, it's a relative minor bugfix/tweak, not something is an important update that someone upgrading should know about.
To that end - this file should:
- Be named
697.misc.md - Only include the text "Some minor errors in ObjC type documentation were resolved.".
src/rubicon/objc/types.py
Outdated
| """Create and return an ``NSEdgeInsets`` with the given values. | ||
| Parameters | ||
| ---------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring for other ...Make() methods is indicative here. We don't need to declare parameters (and if we did, we would use Sphinx notation); and the reference to NSEdgeInserts should be a cross-reference, not a RST literal.
|
Comments actioned. Ready for review |
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tere was one last pre-commit issue caused by a trailing space on a line; I've corrected that, and tests have now passed. Thanks for the PR!
|
Hooray! My first issue |
This update addresses issues raised in #697 :