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

Support rebuilds, add pre-commit #19

Merged
merged 4 commits into from
Aug 30, 2020
Merged

Conversation

chrisjsewell
Copy link
Contributor

Fixes #18

@Daltz333
Copy link
Member

@chrisjsewell I'm aiming to push a release tomorrow night around 10:30PM EST.

@chrisjsewell
Copy link
Contributor Author

I'm aiming to push a release tomorrow night around 10:30PM EST.

Cool cheers

If you don't mind I added a pre-commit config to the repo, and run the black CI via that (there's some other commented out checks you might consider adding)

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Aug 30, 2020

I'm also running the tests via tox.
I can add the tox.ini file if you want?
Basically both pre-commit and tox are super useful for not having to manually set up a development environment; you just run either and it will do it for you

Copy link
Member

@TheTripleV TheTripleV left a comment

Choose a reason for hiding this comment

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

Could you add a dirhtml test case similar to the html test case you already added?

@TheTripleV
Copy link
Member

Sure! I'm open to the tox file.

@chrisjsewell
Copy link
Contributor Author

Could you add a dirhtml test case

Done 👍

I figured out something a bit annoying; sphinx doesn't clean the build folder in-between each test.
Hence, the addition of the shutil.rmtree at the start of these rebuild tests

@TheTripleV
Copy link
Member

Yeah I just noticed that while working on the other open pr. I'll make an issue as we should clean the build folder before every test.

@chrisjsewell
Copy link
Contributor Author

Yeah I just noticed that while working on the other open pr.

Yeh I guess just make a new fixture:

@pytest.fixture()
def app_clean(app):
    if Path(app.outdir).exists():
      shutil.rmtree(Path(app.outdir))
    yield app

(or clean after)

Copy link
Member

@TheTripleV TheTripleV left a comment

Choose a reason for hiding this comment

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

The dirhtml test is using the html builder.

tests/test_ext.py Outdated Show resolved Hide resolved
tests/test_ext.py Outdated Show resolved Hide resolved
Co-authored-by: Vasista Vovveti <vasistavovveti@gmail.com>
@chrisjsewell
Copy link
Contributor Author

The dirhtml test is using the html builder.

Ooops my bad 🤦

@TheTripleV
Copy link
Member

LGTM. This will get merged in tomorrow.

@chrisjsewell
Copy link
Contributor Author

Brilliant, thanks for the great extension 😄

@Daltz333 Daltz333 changed the title Support rebuilds Support rebuilds, add pre-commit Aug 30, 2020
@Daltz333 Daltz333 merged commit 84aa2a3 into wpilibsuite:master Aug 30, 2020
TheTripleV added a commit to TheTripleV/sphinxext-rediraffe-1 that referenced this pull request Sep 30, 2020
* Support rebuilds

* Add pre-commit and CI

* Add dirhtml test

* Apply suggestions from code review

Co-authored-by: Vasista Vovveti <vasistavovveti@gmail.com>

Co-authored-by: Vasista Vovveti <vasistavovveti@gmail.com>
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.

Properly support rebuilds
3 participants