Move Documentation, Pretty class names and auto-deployment #46

TagnumElite · 2017-11-09T19:27:10Z

@niftools/pyffi-reviewer

Overview

Add GitHub Pages documentation deployment
Move docs-sphinx to docs.
Move pyffi*.rst to their respective folders
Update README.rst and CONTRIBUTE.rst
Add build_docs to setup.py
Make PyFFI installable without sphinx
Change setup.py classifiers to a list
Add Missing Documentation Files
Pretty class names of _MetaEnumBase, _MetaStuctBase and _MetaBitStuctBase
Add Favicon/Logo to documenation
Make _MetaEnumBase iterable and subscriptable
Make _MetaEnumBase attributes snake case to be importable

Fixes Known Issues

None

Documentation

Add Sphinx Auto-Documentation:

EGT and ESP Formats
TGA and DDS Spells

Testing

Run command:

    cd docs
    make html
==or==
    python setup.py build_docs

Manual

You will need to set a github_token variable in your Travis project repository environment variables.

Automated

No tests were added or removed. No errors from py.test

Additional Information

The .travis.yml file will need to be updated to suite your repository needs.

- Move docs-sphinx to docs. - Move pyffi*.rst to their respective folders - Update README and CONTRIBUTE - Add build_docs to setup.py - Change setup.py classifiers to a list - Add Missing Documentation Files - Pretty class names of EnumBase, _MetaStuctBase and _MetaBitStuctBase - Add Favicon to documenation - Add githubpages deployment - Make EnumBase iterable and subscriptable - Make EnumBase attributes snake case to be importable

- Make pyffi installable without sphinx.

- .travis.yml missing a step to build documenation

- Add makeshift BuildDoc command - Fix type `extras_require` in setup.py

coveralls · 2017-11-09T20:12:54Z

Coverage decreased (-0.01%) to 58.373% when pulling 80ae401 on TagnumElite:sphinx into 82083af on niftools:develop.

coveralls · 2017-11-09T20:12:54Z

Coverage decreased (-0.01%) to 58.373% when pulling 80ae401 on TagnumElite:sphinx into 82083af on niftools:develop.

coveralls · 2017-11-09T20:12:54Z

Coverage decreased (-0.01%) to 58.373% when pulling 80ae401 on TagnumElite:sphinx into 82083af on niftools:develop.

neomonkeus · 2017-11-10T10:58:29Z

Going to take a look at this more in-depth today. Just took a quick peak at the coverage, bit surprised that there was a decrease. Can you take a look at whether this is needed any longer - https://coveralls.io/builds/14125327/source?filename=pyffi%2Futils%2F__init__.py#L47

neomonkeus · 2017-11-10T14:02:29Z

docs/conf.py

+
+on_rtd = os.getenv('READTHEDOCS') == 'True'
+
+sys.path.insert(0, os.path.abspath('..'))


os.path.dirname(os.path.abspath(file))

os.path.dirname(os.path.abspath(__file__))

otherwise github likes to eat _ to make things bold :/

I also need to delete on_rtd = os.getenv('READTHEDOCS') == 'True' that was only for testing purposes

neomonkeus · 2017-11-10T14:04:44Z

Solid work 👍 Will take a spin at it later.

psi29a · 2017-11-10T18:24:07Z

Agreed! :)

neomonkeus · 2017-11-10T18:42:06Z

pyffi/utils/__init__.py


+class BuildDoc(Command):


Why would we not have this?

That command class is only imported if python can't find sphinx so that pyffi can be installed without sphinx

Sorry, probably meant to say when would occur? Presume your thinking is that failing to build docs should not fail the build.

From an automation perspective the danger here is that we don't get notified if something goes wrong. Should probably take the break it fix it approach. Especially if the intent is to upload them, we don't want to upload broken docs.

I can make it so that instead of print an message it raises an exception. I don’t really know how setup.py works too well so I thought that if the user doesn’t have Sphinx installed it will raise an exception when trying to install PyFFI because we import the Sphinx BuildDoc in the setup.py

Ah, that could be a general problem from either perspective. If a user doesn't want to run docs, they shouldn't need it to install, we should localize scope.

Thats why I added this to the setup.py so that if people do want to run documentation that just have to pip install pyffi[docs] which will also add sphinx to the downloads.

Yeah, still alot more thoughts that I would like to run through with you, but I think at this stage probably best at this stage to take there discussions over on chat as it is cluttering the review. Happy enough to understand why this was done this way and definitely something I would like us to build on. Don't want to hold up the PR any further.

neomonkeus · 2017-11-10T18:45:45Z

setup.py

    long_description=long_description,
    url="https://github.com/niftools/pyffi",
-    download_url="https://github.com/niftools/pyffi/releases"
+    download_url="https://github.com/niftools/pyffi/releases",
+    cmdclass={'build_docs': BuildDoc},


Should we not alias this in setup.cfg

We could in fact move a lot of things inside setup.py into setup.cfg to clean up the setup.py script.
classifiers, packages, package_data and scripts can be put into setup.cfg to save on many lines

Actually was more about creating an actual alias for python setup.py docs. Not sure of what the standard operations/conventions are for setup.py.

@psi29a You're probably more in tune with this as you mapped the test operation to abstact away what underlying test framework we actually use -
https://github.com/TagnumElite/pyffi/blob/80ae40191766622795d36384fac66c020ac00719/.travis.yml#L26

Also we should also consider moving the coverage stuff to setup.cfg as per the discussion around but out of scope of current

I have no problem pushing more things out to setup.cfg, I had no idea the cmdclass trick existed so colour me impressed. You're right, we shouldn't require sphinx in order to use pyffi.

Yeah I was just wondering if there were certain standard command, eg in java land you have maven which has compile, test, install etc. was wondering if there were was similiar in setup.py. Probably should apply some google-fu at this point.

TagnumElite · 2017-11-10T21:00:11Z

@neomonkeus This is because I added a not so well documented BuildDoc command class that is used when sphinx is not installed. This stops python setup.py build_docs/install from throwing errors which could cause appveyor and/or travis from failing if they ever happen to fail to install sphinx. Plus users shouldn't need to install sphinx to use PyFFI

neomonkeus · 2017-11-13T16:58:37Z

pyffi/object_models/xml/enum.py

@@ -84,7 +84,43 @@ def __init__(cls, name, bases, dct):

        # set enum values as class attributes
        for item, value in zip(cls._enumkeys, cls._enumvalues):
-            setattr(cls, item, value)
+            setattr(cls, "".join(snakeCase.name_parts(item)), value)


Missed this on the first run. Eh, not nice, Wonder if we should split out the functionality as it looks to be static rather than class-methody.

Sphinx imports and runs the code to document it. With nif.xml L994 - L1012, the names have spaces in them, with the current code it sets NifFormat.BSLighingShaderPropertyShaderType.Environment Map which can't be imported by normal python code, this fixes it and doesn't break any existing code

I mean how we could the class generation to the names. They should be static methods.

psi29a

LGTM. :)

psi29a · 2017-11-14T20:58:35Z

It is done!

TagnumElite added 4 commits October 30, 2017 14:47

Improve Setup.py

283bc49

- Make pyffi installable without sphinx.

Fix Travis

f1c9b87

- .travis.yml missing a step to build documenation

Change setup.py

80ae401

- Add makeshift BuildDoc command - Fix type `extras_require` in setup.py

neomonkeus requested review from neomonkeus and psi29a November 10, 2017 10:56

neomonkeus self-assigned this Nov 10, 2017

neomonkeus added documentation enhancement labels Nov 10, 2017

neomonkeus added this to the Version 2.2.4 milestone Nov 10, 2017

neomonkeus reviewed Nov 10, 2017

View reviewed changes

neomonkeus reviewed Nov 13, 2017

View reviewed changes

psi29a approved these changes Nov 14, 2017

View reviewed changes

neomonkeus approved these changes Nov 14, 2017

View reviewed changes

neomonkeus merged commit 7f61cc2 into niftools:develop Nov 14, 2017

neomonkeus added this to Done in Pyffi Release Readiness Nov 15, 2017

TagnumElite deleted the sphinx branch November 25, 2017 10:04

TagnumElite mentioned this pull request Nov 26, 2017

Improve/Clean sphinx related code #48

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Documentation, Pretty class names and auto-deployment #46

Move Documentation, Pretty class names and auto-deployment #46

TagnumElite commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017 •

edited

neomonkeus commented Nov 10, 2017 •

edited

neomonkeus Nov 10, 2017

psi29a Nov 10, 2017 •

edited

TagnumElite Nov 10, 2017

neomonkeus commented Nov 10, 2017

psi29a commented Nov 10, 2017

neomonkeus Nov 10, 2017 •

edited

TagnumElite Nov 10, 2017

neomonkeus Nov 13, 2017 •

edited

TagnumElite Nov 13, 2017

neomonkeus Nov 13, 2017

TagnumElite Nov 14, 2017

neomonkeus Nov 14, 2017

neomonkeus Nov 10, 2017

TagnumElite Nov 10, 2017

neomonkeus Nov 13, 2017

psi29a Nov 13, 2017

neomonkeus Nov 13, 2017

TagnumElite commented Nov 10, 2017 •

edited

neomonkeus Nov 13, 2017

TagnumElite Nov 13, 2017

neomonkeus Nov 13, 2017

psi29a left a comment

psi29a commented Nov 14, 2017


		on_rtd = os.getenv('READTHEDOCS') == 'True'

		sys.path.insert(0, os.path.abspath('..'))

Move Documentation, Pretty class names and auto-deployment #46

Move Documentation, Pretty class names and auto-deployment #46

Conversation

TagnumElite commented Nov 9, 2017

Overview

Fixes Known Issues

Documentation

Testing

Manual

Automated

Additional Information

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017 • edited

neomonkeus commented Nov 10, 2017 • edited

Choose a reason for hiding this comment

psi29a Nov 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neomonkeus commented Nov 10, 2017

psi29a commented Nov 10, 2017

neomonkeus Nov 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neomonkeus Nov 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TagnumElite commented Nov 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psi29a left a comment

Choose a reason for hiding this comment

psi29a commented Nov 14, 2017

coveralls commented Nov 9, 2017 •

edited

neomonkeus commented Nov 10, 2017 •

edited

psi29a Nov 10, 2017 •

edited

neomonkeus Nov 10, 2017 •

edited

neomonkeus Nov 13, 2017 •

edited

TagnumElite commented Nov 10, 2017 •

edited