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 the Cargo documentation #87

Merged
merged 10 commits into from
May 12, 2022
Merged

Conversation

Drabble
Copy link
Collaborator

@Drabble Drabble commented May 8, 2022

I have been reading the code for my own comprehension. I took the opportunity to try and improve the overall Cargo documentation of the project. My understanding is still not complete and there may be issues in my comments. I am more than open to criticism and I still need to go through my comments one more time.

I have also added a few FIXME comments where I found potential issues, these I would like to discuss before merging them in the main branch.

There are multiple points to be defined before we can merge:

  • Does it make sense to add simple comments like Shared thread state on the [crate::io:shared_thread_state] module so it looks better in the Cargo documentation? I think it can make sense to have a placeholder in some cases that we can improve when we have more time and knowledge.
  • Which comment convention should we use? I have found this one: Standard library convention.
  • Is it useful to add a period at the end of each phrase in comments?

🚨 Test instructions

Run the following command to open the documentation:

cargo doc -p maplibre --no-deps --open

✔️ PR Todo

  • Validate the comments
  • Review the FIXME comments

@Drabble Drabble added the docu Documentation task label May 8, 2022
@Drabble Drabble requested a review from maxammann May 8, 2022 10:43
@Drabble Drabble self-assigned this May 8, 2022
maplibre/src/map_state.rs Outdated Show resolved Hide resolved
maplibre/src/input/mod.rs Outdated Show resolved Hide resolved
maplibre/src/input/mod.rs Outdated Show resolved Hide resolved
maplibre/src/input/pan_handler.rs Outdated Show resolved Hide resolved
maplibre/src/io/mod.rs Outdated Show resolved Hide resolved
maplibre/src/lib.rs Outdated Show resolved Hide resolved
maplibre/src/lib.rs Outdated Show resolved Hide resolved
maplibre/src/platform/mod.rs Outdated Show resolved Hide resolved
maxammann
maxammann previously approved these changes May 9, 2022
maplibre/src/coords.rs Outdated Show resolved Hide resolved
maplibre/src/coords.rs Outdated Show resolved Hide resolved
maplibre/src/error.rs Outdated Show resolved Hide resolved
maplibre/src/coords.rs Outdated Show resolved Hide resolved
maplibre/src/input/pan_handler.rs Outdated Show resolved Hide resolved
maplibre/src/map_state.rs Outdated Show resolved Hide resolved
maplibre/src/map_state.rs Show resolved Hide resolved
maplibre/src/platform/noweb/http_client.rs Outdated Show resolved Hide resolved
maplibre/src/style/layer.rs Outdated Show resolved Hide resolved
maplibre/src/winit.rs Outdated Show resolved Hide resolved
@maxammann
Copy link
Collaborator

I just merged in a larger PR. Let me know if I should resolve merge conflicts.

Copy link
Collaborator Author

@Drabble Drabble left a comment

Choose a reason for hiding this comment

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

Add more comments and apply suggestions.

maplibre/src/coords.rs Outdated Show resolved Hide resolved
maplibre/src/coords.rs Outdated Show resolved Hide resolved
maplibre/src/input/pan_handler.rs Outdated Show resolved Hide resolved
maplibre/src/io/shared_thread_state.rs Show resolved Hide resolved
maplibre/src/lib.rs Show resolved Hide resolved
maplibre/src/platform/noweb/http_client.rs Outdated Show resolved Hide resolved
maplibre/src/style/layer.rs Outdated Show resolved Hide resolved
maplibre/src/style/layer.rs Outdated Show resolved Hide resolved
maplibre/src/winit.rs Outdated Show resolved Hide resolved
maplibre/src/winit.rs Outdated Show resolved Hide resolved
@maxammann
Copy link
Collaborator

Minor hint: Instead of adding PR comments you can also directly edit files in the PR via the wrench icon :)

maplibre/src/benchmarking.rs Outdated Show resolved Hide resolved
maplibre/src/window.rs Outdated Show resolved Hide resolved
@Drabble Drabble marked this pull request as ready for review May 12, 2022 00:28
maxammann
maxammann previously approved these changes May 12, 2022
maplibre-winit/src/input/tilt_handler.rs Outdated Show resolved Hide resolved
maplibre/src/lib.rs Outdated Show resolved Hide resolved
maxammann
maxammann previously approved these changes May 12, 2022
maplibre-winit/src/input/tilt_handler.rs Outdated Show resolved Hide resolved
maplibre/src/lib.rs Show resolved Hide resolved
@maxammann maxammann enabled auto-merge (squash) May 12, 2022 13:20
@maxammann maxammann merged commit c3edea2 into maplibre:main May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docu Documentation task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants