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

Improve developer documentation and experience #184

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

WardBrian
Copy link
Collaborator

  • Cleans up CONTRIBUTING.md to push even more information into the online/rendered documentation
  • Improves the makefile error provided when the submodules are missing (similar to Improve missing submodule logic stan-dev/cmdstan#1217)
  • Added a makefile target to format all code
  • Made the .pylintrc in python/ a bit more useful for our project (we still don't enforce this anywhere)

@WardBrian WardBrian requested a review from roualdes November 6, 2023 16:37
Copy link
Owner

@roualdes roualdes left a comment

Choose a reason for hiding this comment

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

One suggestion, one question, and otherwise looks good. Thanks.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@WardBrian WardBrian merged commit 9fecdb3 into main Nov 6, 2023
@WardBrian WardBrian deleted the doc/contributing-cleanup branch November 6, 2023 20:12
Copy link
Collaborator

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

Just a few comments on doc. I like all the changes to Python that move things up out of else conditions when the main if just fires off an exception that will reroute control.

CONTRIBUTING.md Outdated
@@ -2,7 +2,9 @@

We welcome contributions to the project in any form, including bug reports, bug fixes, new features, improved documentation, or improved test coverage.

Developer-specific documentation is available at https://roualdes.github.io/bridgestan/latest/internals.html
**Developer-specific documentation is available at [as part of our online documentation](https://roualdes.github.io/bridgestan/latest/internals.html)**. Information on building BridgeStan, running tests, and developing with the project is available there.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer links to be minimal nouns or noun phrases, so I would use as part of our [online documentation](...), though it could be as part of [our online documentation](...) or even as [part of our online documentation](...).

Developer-specific documentation is available at https://roualdes.github.io/bridgestan/latest/internals.html
**Developer-specific documentation is available at [as part of our online documentation](https://roualdes.github.io/bridgestan/latest/internals.html)**. Information on building BridgeStan, running tests, and developing with the project is available there.

This document houses additional information about licensing and procedures for contributing to the project.
Copy link
Collaborator

Choose a reason for hiding this comment

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

houses -> contains OR supplies OR provides, because there's no housing, so it's distracting.

C++
---

* We use the C++1y standard for compilation (``-std=c++1y`` in both clang and gcc). This is partway between C++11 and C++14, and is what Stan requires.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As of R 4.3, they support C++17. It rolled out earlier this year with a late 4.2.x. See https://developer.r-project.org/blosxom.cgi/R-devel/NEWS/2023/01/27#n2023-01-27

I don't know if Python is a blocker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we don't rely on any specific C++ ABI, we could use any C++ standard we like for this project. This comment is just there because it's what our makefiles pass and what the rest of Stan uses


* We try to follow the `Google C++ Style Guide <https://google.github.io/styleguide/cppguide.html>`_, but (a) we allow C++ exceptions, and (b) we allow reference arguments.

* We recommend using `Clang format <https://clang.llvm.org/docs/ClangFormat.html>`_ with our config file `.clang-format <https://github.com/roualdes/bridgestan/blob/main/.clang-format>`_.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line's too long. I think it's OK to wrap even with bullets.

docs/internals/development.rst Show resolved Hide resolved
@bob-carpenter
Copy link
Collaborator

Sorry--I didn't see @roualdes approving this or I wouldn't have added other picky comments (though I would still prefer to see the syntax of what's included in links be a noun).

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