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

docs: general formatting, fix links, cleanups, rewrites #554

Merged
merged 10 commits into from
Oct 13, 2020

Conversation

chuck-sys
Copy link
Collaborator

@chuck-sys chuck-sys commented Oct 12, 2020

Details

  • Add links to types in internal reference
  • Change the unicode directional single-/double-quotes to ascii ones so
    that mac users aren't selectively favoured
  • Add even more documentation on less documented functions
  • Make factory documentation visible when it wasn't for so long
  • Rewrite segments of architecture because it no longer reflected the
    current state of the project
  • Reformat some names to make internal reference links possible

View newly built docs here: https://rocket2.readthedocs.io/en/cheukyin699-update-docs/

- Add links to types in internal reference
- Change the unicode directional single-/double-quotes to ascii ones so
that mac users aren't selectively favoured
- Add even more documentation on less documented functions
- Make factory documentation visible when it wasn't for so long
- Rewrite segments of architecture because it no longer reflected the
current state of the project
- Reformat some names to make internal reference links possible

Probably should be in separate commits. But they are all under the
umbrella of general cleanup.
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #554 into master will not change coverage.
The diff coverage is 85.71%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #554   +/-   ##
=======================================
  Coverage   93.81%   93.81%           
=======================================
  Files          47       47           
  Lines        2669     2669           
  Branches      361      361           
=======================================
  Hits         2504     2504           
  Misses        108      108           
  Partials       57       57           
Impacted Files Coverage Δ
app/controller/command/commands/karma.py 93.02% <ø> (ø)
app/controller/command/commands/project.py 96.95% <ø> (ø)
app/controller/command/commands/team.py 86.35% <ø> (ø)
app/controller/command/commands/user.py 98.93% <ø> (ø)
app/controller/webhook/github/core.py 100.00% <ø> (ø)
app/model/team.py 100.00% <ø> (ø)
app/model/user.py 100.00% <ø> (ø)
config/__init__.py 100.00% <ø> (ø)
db/dynamodb.py 90.51% <ø> (ø)
db/facade.py 100.00% <ø> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07e20dc...c451e3e. Read the comment docs.

@chuck-sys chuck-sys marked this pull request as ready for review October 12, 2020 20:50
@chuck-sys chuck-sys requested review from a team as code owners October 12, 2020 20:50
@bobheadxi
Copy link
Member

Change the unicode directional single-/double-quotes to ascii ones so
that mac users aren't selectively favoured

❤️

@@ -635,7 +639,8 @@ def refresh_helper(self, user_id) -> ResponseTuple:
Ensure that the local team database is the same as GitHub's.

In the event that our local team database is outdated compared to
the teams on GitHub, this command can be called to fix things.
the teams on GitHub, this command can be called to fix these
inconsistencies.
Copy link
Member

Choose a reason for hiding this comment

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

This likely ties into #490 in that we can't remove this command, as we'll always need something to forcibly bring everything in line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably need to add a Deprecation Warning whenever someone calls the command.

Copy link
Member

Choose a reason for hiding this comment

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

Oh my point was I dont think we can deprecate it, ever, since we need some way to force a state sync (for example, to add new features - essentially a "migration")

# The full version, including alpha/beta/rc tags
release = '1.0.0'
release = '2.0.0'
Copy link
Member

Choose a reason for hiding this comment

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

do we even do releases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. Actually, there is a slim chance that this would come back and bite us. Technically we do releases. We are currently on v2.1.0. I'm thinking that'd be one of the things we'd talk about in the meeting?

Gonna change this to better reflect, but right now docs are generated (and not saved) on master push, soooo rolling release.

Copy link
Member

Choose a reason for hiding this comment

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

I dropped an item in the meeting notes - feel free to add stuff! https://docs.google.com/document/d/1KcH2l6nJKyHoCDe6v6Sl0cLWQWKQQmSLqk_kXvZUsRw/edit

@@ -118,7 +118,7 @@ def __init__(self, missing_config_fields):
"""
Initialize a new MissingConfigError.

:param: missing_config_fields List of missing config variables
:param missing_config_fields: the missing config variables
Copy link
Member

Choose a reason for hiding this comment

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

is there a linter that can catch these documentation errors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I doubt it.

docs/Architecture.rst Outdated Show resolved Hide resolved
@bobheadxi
Copy link
Member

This might be because, to center-align the "Rocket 2" header in both GitHub README and the website, I use HTML, which isn't recognized by Sphinx as the top-level header for the page - is there a way to override this? (should be "Rocket 2", not "Usage")

image

Cheuk Yin Ng and others added 3 commits October 12, 2020 18:37
Co-authored-by: Robert Lin <[email protected]>
- set header of readme correctly so that it is picked up by sphinx
- use links to make README slightly more readable
@chuck-sys
Copy link
Collaborator Author

chuck-sys commented Oct 13, 2020

Is there a way to override this?

I just noticed. Good question. After a bit of read, I think it's safe to say that, yes, there is a way to override this. And no, it's not pretty. See 62a6e82.

It involves creating a dummy header in rst that gets turned invisible by CSS. Normally you can center paragraphs using the .. rst-class:: center directive, which pins the .center class to an element. However, if you do that on a header, it pins the .center class to the entire section (header + everything until the next header). More accurately, it adds the .center class to a div that encompasses the entire section - header and paragraphs below (until the next header). The hack I use has the Rocket 2 section right next to the Usage section so that it ends immediately. That way I can make it invisible using a class (which makes the entire section invisible), but Sphinx reads the section and thinks that the first header still exists.

The raw HTML is still necessary for centering the header up top. At least, some of it is.

@bobheadxi
Copy link
Member

bobheadxi commented Oct 13, 2020

@cheukyin699 sadly, your approach in 62a6e82 doesn't work on GitHub :( that's why I made the entire top section raw HTML previously - so that it would work on GitHub as well (IMO this is a requirement) - how it should look

image

@chuck-sys
Copy link
Collaborator Author

@rob ahhh. Ohhhh. Welp, back to the drawing board. Might not be possible then.

@bobheadxi
Copy link
Member

Let's leave that for another PR - can you revert 62a6e82 and I'll approve?

@chuck-sys
Copy link
Collaborator Author

Another option:

.. raw:: html

	<style>
		h1:nth-of-type(2) {
			display: none;
		}
	<style>

But that would be way too hacky for me.

@chuck-sys
Copy link
Collaborator Author

@bobheadxi reverted

Copy link
Member

@bobheadxi bobheadxi left a comment

Choose a reason for hiding this comment

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

thank you!!!!

@chuck-sys chuck-sys merged commit 1fa4fa5 into master Oct 13, 2020
@chuck-sys chuck-sys deleted the cheukyin699/update-docs branch October 13, 2020 03:44
@bobheadxi
Copy link
Member

Created #555 as follow-up

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.

2 participants