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

Documentation Enhancements #1424

Merged
merged 20 commits into from
Dec 28, 2018

Conversation

harshanarayana
Copy link
Contributor

@harshanarayana harshanarayana commented Dec 7, 2018

  • - Document missing APIs
  • - Enhance Example Section in Document
  • - Categorize Extensions based on groups (Suggestions on the group order is welcome)
  • - Fix minor formatting issues
  • - Release manager script with Calendar Verison and Micro release support.
  • - Add examples with External monitoring and error reporting service integration. (Sentry, Rollbar, Raygun and Logdna)
  • - Release versioning script compliant with Black
  • - Minor tweaks to the documentation with formatting and text contents
  • - Reordered index in the document to simplify the use for first-time users.
  • - Run Tests using setup.py
  • - Additional Makefile commands for testing and Coverage Report

Note: There are still a few more sanic classes that need API documentation. I will go ahead and create a different PR for them.

@codecov
Copy link

codecov bot commented Dec 7, 2018

Codecov Report

Merging #1424 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1424      +/-   ##
==========================================
+ Coverage   91.26%   91.32%   +0.05%     
==========================================
  Files          17       17              
  Lines        1717     1717              
  Branches      322      322              
==========================================
+ Hits         1567     1568       +1     
  Misses        123      123              
+ Partials       27       26       -1
Impacted Files Coverage Δ
sanic/router.py 95.89% <ø> (+0.45%) ⬆️
sanic/handlers.py 98.09% <ø> (ø) ⬆️
sanic/exceptions.py 100% <ø> (ø) ⬆️
sanic/cookies.py 100% <ø> (ø) ⬆️
sanic/app.py 91.21% <100%> (ø) ⬆️

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 67d51f7...9bea23d. Read the comment docs.

@harshanarayana harshanarayana changed the title [WIP] Documentation Enhancements Documentation Enhancements Dec 8, 2018
ahopkins
ahopkins previously approved these changes Dec 9, 2018
Copy link
Member

@ahopkins ahopkins 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.

@harshanarayana
Copy link
Contributor Author

harshanarayana commented Dec 9, 2018

@ahopkins @vltr @yunstanford I just added a basic Release management script that can automatically manage calendar version with micro-release support and change log generation for release note. I would really appreciate it if you guys could take a look and provide a feedback on that script.

@ahopkins
Copy link
Member

ahopkins commented Dec 9, 2018

Will do. I also want to make some edits to the README, and the intro page. I was thinking I'd do a separate PR, but maybe I will commit to this one.

@harshanarayana
Copy link
Contributor Author

@ahopkins Great. Let me know if there is something I can add to the README from my side to simplify your work.

@yunstanford
Copy link
Member

awesome, thanks!

sanic/blueprints.py Show resolved Hide resolved
@@ -0,0 +1,221 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if everyone would agree with me, but this could be inside a .ci directory of some kind, specially if ignored by the MANIFEST.in file. Looking at the MANIFEST.in file now, I actually see that we have some work to do in there as well ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vltr I like the idea of keeping all CI/CD tools in a common directory so that they are out of the way of regular development.

As for the MANIFEST.in goes, I was thinking of doing it as a different PR. But I can change it as part of this one as well if you want me to.

Just an FYI, check out https://flit.readthedocs.io/en/latest/

Copy link
Member

Choose a reason for hiding this comment

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

@harshanarayana yeah, there are some tools today that can do that for us ... There is towncrier to automate the changelog entries (you can see it in use in the attrs project). There's poetry that can handle some of the same stuff that flit does, but I actually have never used them and, while the eternal discusion over a new tool that "does everything in packaging" isn't over with a consensus, I guess we'll stick to setuptools for a while ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vltr towncrier seems interesting. I didn't know about it before you mentioned it here.
I was thinking of making the necessary changes to the MANIFEST.in fie. Do you have any suggestions on what might be good to go there?
I was thinking the following.

  1. Documentations (Raw files not the _build)
  2. Setup
  3. Tests
  4. yml, md, rst, license, txt, ini from the base directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

➜ check-manifest -u -v
listing source files under version control: 185 files and directories
building an sdist: sanic-0.8.3.tar.gz: 185 files and directories
copying source files to a temporary directory
building a clean sdist: sanic-0.8.3.tar.gz: 185 files and directories
lists of files in version control and sdist match

Manifest Coverage after new changes to manifest

Copy link
Member

Choose a reason for hiding this comment

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

Wow! Are you trying to do all the hardwork by yourself? 😅

Indeed, towncrier seems like a nice tool to provide a complete changelog and that kind of stuff, even though I have not used it myself (yet).

As for the MANIFEST.in file, I think that the wheel distribution should have as little as possible (IMHO). I would exclude almost everything except for code, LICENSE and the setup.py. But that's just my opinion. I don't know if we can make different packages for wheel and sdist. For sdist I would just add everything that's not related to our development process, leaving mostly files like setup.cfg (it just holds information for dev as well), tox.ini, .travis.yml and etc out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vltr I did a few tests with the new MANIFEST.in file and found the following

  1. whl files only have the bare minimum contents as expected
  2. sdist generates an uber tar with all the contents specified from this list.

I am yet to find out a way to explicitly configure what goes into whl and what goes into sdist.

Copy link
Member

@vltr vltr Dec 17, 2018

Choose a reason for hiding this comment

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

That's interesting. I would expect the whl file to have the minimum contents, I just wanted to make sure it doesn't includes examples and tests (all containing .py files that, well, might get inside for some reason unknown). Now, the sdist I think would require the minimum to execute python setup.py install, the LICENSE and that would be it 😉

@harshanarayana harshanarayana force-pushed the enh/Documentation_Update branch from f629521 to cba4a3e Compare December 15, 2018 11:14
MANIFEST.in Show resolved Hide resolved
@vltr
Copy link
Member

vltr commented Dec 15, 2018

I have some docs to add myself to Sanic (so I can close #1329), but I think I'll just wait yours to be merged first because, well, it's almost like a complete overhaul 💪

MANIFEST.in Outdated Show resolved Hide resolved
@harshanarayana harshanarayana force-pushed the enh/Documentation_Update branch 2 times, most recently from 881e184 to 9eedfbc Compare December 18, 2018 16:49
ujson,
'pytest-sanic',
'pytest-sugar'
]
Copy link
Member

Choose a reason for hiding this comment

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

There are three requirement files, maybe load requirements from those files would be easier to maintain dependencies of Sanic?(cause we only have to focus on requirement files)
BTW, it seems that you change the way to run tests, this might also need to be documented at the contributing guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chenjr0719 I like your suggestion. I need to clean up the requirements-dev.txt a bit before I can read and use it inside the setup.py. Let me get that sorted.

Btw, this doesn't alter the way to run tests. Just provides an extra way to run the test if you get the tarball from sdist and you want to run make test before you install sanic in your machine/environment. This is more of a consumer-facing feature than dev facing change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chenjr0719 IMHO, I would love it if we can totally get rid of the requirements*.txt file and use extras_require for additional dependencies and tests_require for test related dependencies.

I am open to suggestion on this matter. I think not having a requirements*.txt file is much better option,

Copy link
Member

Choose a reason for hiding this comment

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

@harshanarayana To be honest, I would prefer to use requirements*.txt files to manage my environment. That's why I propose this idea. But, after taking a look around some projects, use extras_require is also a good practice. Hum..., it hard to say which one is better or not cause this might also be influenced by the developer's habit.

Anyway, no matter using requiements*.txt files or use extras_require, please explain the usage at README.md or CONTRIBUTING.md.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, for running test, i'd prefer tox to create test virtualenv with proper dependencies, or other tools, instead of setup.py/setuptools directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vltr Outside the .. code:: blocks. It's the new section I am trying to add that is causing this problem.
image
image

Copy link
Member

Choose a reason for hiding this comment

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

Have you tried \' or \" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried that. No luck with that either.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm ... Why don't you do this instead?

``pip3 install -e '[.docs]'``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vltr 👍 Ah this was a much nicer way to get around the problem. It even makes the document look nicer.

@vltr
Copy link
Member

vltr commented Dec 21, 2018

Since you're in the vibe ... Have you seen that there's different installation instructions in the project's README and in the docs?

@harshanarayana
Copy link
Contributor Author

Since you're in the vibe ... Have you seen that there's different installation instructions in the project's README and in the docs?

Done. Fixed it across to be consistent and have the same details.

@vltr
Copy link
Member

vltr commented Dec 22, 2018

Nice!! This is awesome! Thanks, @harshanarayana !

@harshanarayana harshanarayana force-pushed the enh/Documentation_Update branch from 7ec43b6 to 64a99af Compare December 23, 2018 02:43
ahopkins
ahopkins previously approved these changes Dec 24, 2018
@vltr
Copy link
Member

vltr commented Dec 24, 2018 via email

@pypycoder
Copy link

cool, good improvement.

ahopkins
ahopkins previously approved these changes Dec 27, 2018
@vltr vltr mentioned this pull request Dec 27, 2018
@yunstanford
Copy link
Member

@harshanarayana Can you update this branch due to conflicts? will approve and merge in if no one objects.

harshanarayana and others added 19 commits December 28, 2018 10:22
Signed-off-by: Harsha Narayana <[email protected]>
Signed-off-by: Harsha Narayana <[email protected]>
Signed-off-by: Harsha Narayana <[email protected]>
@harshanarayana harshanarayana force-pushed the enh/Documentation_Update branch from a3c3281 to 9bea23d Compare December 28, 2018 04:54
@harshanarayana
Copy link
Contributor Author

@yunstanford Rebased and resolved the conflict.

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.

6 participants