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

sphinx build errors #696

Merged
merged 1 commit into from
Feb 4, 2015
Merged

sphinx build errors #696

merged 1 commit into from
Feb 4, 2015

Conversation

steenzout
Copy link
Contributor

Hello,

this PR is to make sphinx doc generation warnings and link check break the build if something gets changed on the docs that isn't right.

I had to:

  • change the import for sqlalchemy in the luigi/db_task_history.py so that sphinx wouldn't complain (hope it doesn't look that ugly!)

  • fix all the warnings on the docstrings

  • fix URLs so that permanent redirects don't throw sphinx errors anymore

  • had to make a copy of the README.rst into doc/readme.rst since links to external images / figures will throw a sphinx warning during the documentation build (is this ok? at least, for now?)

  • 2 minor sphinx configuration tweaks to fix / bypass / overcome sphinx warnings

  • added docs-test tox environment

  • added docs-test tox environment to the travis build

    I also added a few changes to the .gitignore.
    Based on the content you previously had I:

  • generated new rules on gitignore.io and added them.

  • removed what became duplicate from the top of the file

  • ordered the project custom rules by alphabetical order

  • added a few python files that show up after I run the tests

    let me know if you want me to rebase all of this into 1 commit (some projects don't like a series of commits).

    comments welcomed!...

cheers

deps =
sqlalchemy
Sphinx
sphinx_rtd_theme
changedir = doc
commands = /usr/bin/make {posargs}
commands = /usr/bin/make SPHINXOPTS='-W' {posargs}
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we use sphinx-build instead? #697

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw the PR.
either way works.
I'll wait for the merge for #697 and then I'll rebase.

@erikbern
Copy link
Contributor

erikbern commented Feb 3, 2015

Looks awesome! Except

  1. Not great with two README files that go out of sync
  2. Can you squash a bunch of the commits? I prefer it to be just a handful

@steenzout
Copy link
Contributor Author

@erikbern changes squash for 2.

regarding 1. I've added a note.
I'm not sure how to not dup it.

@erikbern
Copy link
Contributor

erikbern commented Feb 3, 2015

I don't think we need to syntax check README.rst since you can look at it by just going to the branch on Github. Can we just avoid checking it?

@themalkolm
Copy link
Contributor

@steenzout Maybe add a copy command in [docs] to copy README.rst to doc before docs being run? :) No duplication, everybody is happy.

@erikbern
Copy link
Contributor

erikbern commented Feb 3, 2015

+1

@steenzout
Copy link
Contributor Author

@themalkolm if I copy it, sphinx will try to validate it when doing HTML docs generation just like when it was pointing to the ../README.rst. :)

I can add a command to copy it then and perform a cleanup but next time some adds something to the main README.rst it may break the docs generation.
I guess that's ok.

give me 1h...

@themalkolm
Copy link
Contributor

I would say it is OK to break documentation if README.rst is broken.

@themalkolm
Copy link
Contributor

Maybe adding [docs] to default targets to tox would ensure docs are never borked.

@Tarrasch
Copy link
Contributor

Tarrasch commented Feb 4, 2015

This commit message is unreadable since the header does not describe the overall change. The imporpant change of this patch set is not that you ignore E308.

@steenzout
Copy link
Contributor Author

@Tarrasch I've changed the 1st line of the commit message.
hope this is what you wanted or else just let me know.

@themalkolm I've merged the docsapi tox env with the docs tox env since it didn't made any sense to have it separated (I didn't realize that you had to build the docs in 2 steps: api and then HTML).
I've noticed that due to this a luigi.tools.rst file was generated by sphinx.
since it wasn't being included it complained and I fixed it.
please check if on the API Reference it's where you wanted it to be.

to sync the README.rst I copy the file over, change github URL to local file references and remove the badges using sed commands.
sphinx is now happy.

if you really want the badges on the docs (?) we'd have to copy the badges every time we try to generate the documentation and be sure you're picking up the badge for the right branch / tag.

@erikbern
Copy link
Contributor

erikbern commented Feb 4, 2015

Great! Maybe just squash everything since the first commit is already a monster-commit

@steenzout
Copy link
Contributor Author

squash done.
made sed compatible for ubuntu and macosx.

sphinx-apidoc -o doc/api -T luigi

# sync README.rst file
cp README.rst doc/README.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you delete doc/readme.rst since you're copying README.rst into doc/README.rst now?

Also Sphinx is throwing some errors. Could it be because you need to copy README.rst into doc/readme.rst (lower case) rather than doc/README.rst?

Warning, treated as error: /home/travis/build/spotify/luigi/doc/readme.rst:: WARNING: document isn't included in any toctree

ignore autopep8 E309 since if class has docstring, it will be separated by blank line.

changed sqlalchemy import statements.

this is necessary to fix bug in sphinx-doc issue#1530:
sphinx-doc/sphinx#1530

fixed luigi.interface.Interface.run sphinx complaints.
improved luigi.interface.run docstring.

fixed luigi.lock.getpcmd sphinx complaints.

fixed luigi.contrib.mysqldb.MySqlTarget docstring complaints and made 1st character of parameter description lowercase.

fixed luigi.contrib.redshift.RedshiftManifestTask sphinx complaints.

fixed sphinx SEVERE: Title level inconsistent.

fixed sphinx "ERROR: Unknown target name" errors.

fixed sphinx "WARNING: Explicit markup ends without a blank line" errors.

fixed sphinx "WARNING: Explicit markup ends without a blank line" errors.

fixed sphinx "Duplicate explicit target name: 'spotify'" error.

fixed broken URL to Yarn REST API.

fixed URL to oozie.

fixed URL for luigi-user Google group.
fixed URL to spotify.

changed URL to source code to make sphinx linkcheck pass.

added new readme.rst to exclude_pattern to fix sphinx complaint that it's not part of a table of content.
removed the html_static_path to fix sphinx build issue.

sphinx warnings now make the build break.

added docs-test tox environment to the travis build.
problems with documentation will now make the build break.

added more rules to .gitignore based on gitignore.io: Python, vim, OSX, Vagrant, PyCharm.

fixed sphinx warning to include luigi.tools.

merge tox docsapi with tox docs.
tox docsapi environment creates new files that might break the documentation generation so  moved the command into the tox docs environment.

synchronize README.rst in github and readthedocs.

copy of README.rst to doc directory.
URLs pointing to github changed to point to local files.
badges removed from the doc/README.rst.
added comments to build doc commands.
whitelist cp and sed commands.

README.rst added to sphinx exclude_patterns.
this will bypasses the warning sphinx gives due to the fact that README.rst is not part of a table of contents.
@steenzout
Copy link
Contributor Author

@erikbern I saw that.
It worked on my laptop but I was missing a commit to delete the readme.rst (lowercase) file.
I've fixed that now.
it should fix both errors.

sorry about the extra spam!

@erikbern
Copy link
Contributor

erikbern commented Feb 4, 2015

awesome!

erikbern pushed a commit that referenced this pull request Feb 4, 2015
@erikbern erikbern merged commit 9c922ae into spotify:master Feb 4, 2015
@erikbern
Copy link
Contributor

erikbern commented Feb 4, 2015

this was commit #1,500 :)

@steenzout
Copy link
Contributor Author

:)

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.

4 participants