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

Improved internal and external documentation #419

Merged
merged 6 commits into from
Oct 2, 2020
Merged

Improved internal and external documentation #419

merged 6 commits into from
Oct 2, 2020

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Oct 1, 2020

Changes:

  • Added documentation containing type information for a large number of macros
  • Cleans up doc attributes so stardoc renders cleaner
  • Adds new pages to the docs/update_docs.sh output
  • Rearranged a few macros to reduce code duplication
  • Fixed some incompatibility warnings/buildifier defects

@acmcarther @mfarrugi @damienmg @smklein @dfreese
I would hugely appreciate if all code owners would take a look at 548d62f and provide suggestions on how the documentations can be improved. I'm very bad at writing docs and this was largely a learning exercise for me so some of the docs may not be as clear as they should be. There are a few I was totally unsure of what to write (namely get_compilation_mode_opts, _determine_lib_name, rust_toolchain, and RustProtoInfo).

Again, It'd help out tremendously to have these docs updated. Looking forward to suggestions 😄

edit: The commit of the big request for suggestions has been moved to #420

Copy link
Collaborator

@damienmg damienmg left a comment

Choose a reason for hiding this comment

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

Comment by change:

  • Renamed RustProtoProvider -> RustProtoInfo: I don't see the use of this change, Provider is a more common name and I find it more explicit
  • Partially addressed legacy provider syntax issue: LGTM
  • Renamed AliasableDep -> AliasableDepInfo: Can we rename as AliasedDepProvider instead?
  • Renamed relative_path -> relativize: Some nit inlined
  • Moved get_lib_name to //rust:private/utils.bzl: This change is adding an unused function, drop?
  • Updated copyright: Please do not do this, we should not update existing Copyright lines.
  • _Moved determine_output_hash so it can be used in other places: LGTM
  • Updating documentation: this change is too big, please extract to a separate PR.
  • Added more pages to the outward facing docs: Some nit inlined
  • Regenerate documentation: LGTM

cargo/cargo_build_script.bzl Outdated Show resolved Hide resolved
cargo/cargo_build_script.bzl Outdated Show resolved Hide resolved
cargo/cargo_build_script.bzl Outdated Show resolved Hide resolved
rust/private/clippy.bzl Outdated Show resolved Hide resolved
docs/all.bzl Outdated Show resolved Hide resolved
rust/private/utils.bzl Outdated Show resolved Hide resolved
rust/private/utils.bzl Show resolved Hide resolved
@UebelAndre
Copy link
Collaborator Author

* **Renamed RustProtoProvider -> RustProtoInfo**: I don't see the use of this change, Provider is a more common name and I find it more explicit

Buildifier claims this violates naming conventions: https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#name-conventions

@UebelAndre
Copy link
Collaborator Author

* **AliasableDepInfo**: Can we rename as `AliasedDepProvider` instead?

Again, this violates https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#name-conventions

@UebelAndre
Copy link
Collaborator Author

* **Moved get_lib_name to //rust:private/utils.bzl**: This change is adding an unused function, drop?

If it was unused, I would have removed it. It's used in rustc.bzl and rustdoc_test.bzl

@damienmg
Copy link
Collaborator

damienmg commented Oct 2, 2020

* **Renamed RustProtoProvider -> RustProtoInfo**: I don't see the use of this change, Provider is a more common name and I find it more explicit

Buildifier claims this violates naming conventions: https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#name-conventions

Meh :/ This is a bad convention, anyway that's the convention so this change LGTM.

* **Moved get_lib_name to //rust:private/utils.bzl**: This change is adding an unused function, drop?

If it was unused, I would have removed it. It's used in rustc.bzl and rustdoc_test.bzl

This change only add the function without any call site. Unless the code was broken before this method is not used anywhere. You probably forgot to update the load statements and remove the original place of the code.

@UebelAndre
Copy link
Collaborator Author

This change only add the function without any call site. Unless the code was broken before this method is not used anywhere. You probably forgot to update the load statements and remove the original place of the code.

Ah I see, I've updated this now. I didn't completely break out this change from the larger one. This has been updated

@UebelAndre
Copy link
Collaborator Author

* Please do not do this, we should not update existing Copyright lines.

Dropped but can you update it then?

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Oct 2, 2020

* **Updating documentation**: this change is too big, please extract to a separate PR.

Moving to another PR.

edit: See #420

@damienmg
Copy link
Collaborator

damienmg commented Oct 2, 2020

* Please do not do this, we should not update existing Copyright lines.

Dropped but can you update it then?

We should not touch to the copyright line. The date should be the date of creation of the file as per guidance from the opensource office at google.

Copy link
Collaborator

@damienmg damienmg left a comment

Choose a reason for hiding this comment

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

Just one nit left.

rust/private/utils.bzl Outdated Show resolved Hide resolved
@damienmg damienmg merged commit 169476f into bazelbuild:master Oct 2, 2020
@UebelAndre UebelAndre deleted the internal-docs branch October 2, 2020 14:18
@UebelAndre UebelAndre mentioned this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants