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

Remove duplicate README.rst documentation and add links to ReadTheDocs documentation #918

Merged
merged 24 commits into from
Sep 25, 2020

Conversation

pmantica1
Copy link
Contributor

@pmantica1 pmantica1 commented Sep 10, 2020

In this diff, I remove all of the duplicate README.rst documentation and add links to ReadTheDocs.

Note:
We currently have a free ReadTheDocs account. If we ever decide to move to a ReadTheDocs business account then we will be given a different domain, (something like kensho-graphql-compiler.readthedocs.com). I already spoke with the ReadTheDocs support and they said that they can help us keep the current graphql-compiler.readthedocs.io domain as well if we make this transition. So the links I've included in this pr won't become invalid if we do decide to move to a business account.

Also, I did not really find much benefit to having a business account besides the ability to have one unique Kensho account for managing the ReadTheDocs websites of all the Kensho repositories. The compiler is currently the only Kensho repo that is integrated with ReadTheDocs though.

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #918 into main will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #918      +/-   ##
==========================================
+ Coverage   95.02%   95.04%   +0.01%     
==========================================
  Files         109      109              
  Lines        8732     8739       +7     
==========================================
+ Hits         8298     8306       +8     
+ Misses        434      433       -1     
Impacted Files Coverage Δ
graphql_compiler/typedefs.py 100.00% <0.00%> (ø)
graphql_compiler/schema/__init__.py 100.00% <0.00%> (ø)
graphql_compiler/compiler/emit_sql.py 98.49% <0.00%> (ø)
graphql_compiler/compiler/metadata.py 99.13% <0.00%> (ø)
graphql_compiler/macros/validation.py 100.00% <0.00%> (ø)
graphql_compiler/compiler/validation.py 100.00% <0.00%> (ø)
graphql_compiler/compiler/expressions.py 92.07% <0.00%> (ø)
graphql_compiler/query_formatting/common.py 97.87% <0.00%> (ø)
graphql_compiler/cost_estimation/analysis.py 99.04% <0.00%> (ø)
graphql_compiler/query_pagination/__init__.py 100.00% <0.00%> (ø)
... and 13 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 ac66f65...93771c9. Read the comment docs.

Copy link
Contributor

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Would love @JulianGoetz and @branen's eyes on this.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@pmantica1 pmantica1 marked this pull request as ready for review September 23, 2020 23:14
bojanserafimov
bojanserafimov previously approved these changes Sep 24, 2020
Copy link
Collaborator

@branen branen left a comment

Choose a reason for hiding this comment

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

Nice work! I've suggested a few local edits; otherwise, it looks good.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
docs/source/index.rst Show resolved Hide resolved
docs/source/index.rst Outdated Show resolved Hide resolved
docs/source/index.rst Show resolved Hide resolved
docs/source/index.rst Show resolved Hide resolved
docs/source/index.rst Show resolved Hide resolved
Co-authored-by: branen <6373030+branen@users.noreply.github.com>
pmantica1 and others added 3 commits September 24, 2020 14:43
Co-authored-by: branen <6373030+branen@users.noreply.github.com>
Co-authored-by: branen <6373030+branen@users.noreply.github.com>
Copy link
Collaborator

@branen branen left a comment

Choose a reason for hiding this comment

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

Looks good!

@pmantica1 pmantica1 merged commit ab9c470 into main Sep 25, 2020
@pmantica1 pmantica1 deleted the move-to-readthedocs branch September 25, 2020 14:47
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.

5 participants