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

Dev Tool Updates #379

Merged
merged 48 commits into from
Oct 27, 2021
Merged

Dev Tool Updates #379

merged 48 commits into from
Oct 27, 2021

Conversation

MJJoyce
Copy link
Member

@MJJoyce MJJoyce commented Oct 26, 2021

This is part 1 of part [number > 1] of updates for #364. The changes here cover the vast majority of the updates necessary to expand our dev tooling for Core. There's some changes in here related to updating our packaging but I'm more fully in a follow on PR. See the "work to do" list below for what's missing here.

What's here

  • Move our testing over to pytest, update the test suite as necessary, and drop nose.
  • Add black and run a reformatting of the entire code base
  • Add flake8 and useful config, cleanup all linting errors in the code base.
  • Add mypy and attempt to address issues in the code base that is raises. A non-trivial amount pop up because of the dynamic handling that we have for our logging and config. We'll have to figure out a nicer way to address that.
  • Add pre-commit config that executes our entire linting pipeline on commit and includes YAML dictionary validation and test execution on push.
  • Development dependencies are now all installed off the dev extra_require

Work to Go

  • Documentation updates covering these new tools. I'm planning on pulling a good bit of the info we have out of random wiki pages and into the README for easier access.
  • Update our packaging for PEP 517/518 compliance.
  • Evaluate alternative "package managing" tools like poetry

!!! NOTE !!!
Some of the changes here represent backwards incompatible changes due to API name changes. Could we have marked all these issue as noqa and dealt with them later? Yes. But, it needs done eventually and I was already there fixing them. Might as well bite the bullet and get it over with.

Drop `nose` from package requirements and update all tests to be
compatible with `pytests`. Changes for compatibility are pretty minor. For
the most part all that was necessary is updating some `nose` specific
asserts to the `pytest` equivalent. There were some issues with
configuration being corrupted by test modules that didn't adequately
clean up after execution. I assume this issue pops up due to the way
`pytest` executes tests compared to `nose`.

Note, we should work on updating all our tests to use the
`__init__.TestFile` class for automatically dealing with temporary
files. Our current approaches are a bit all over the place at the
moment.
Move all optional / development dependencies under `dev` instead of
splitting it across `tests` and `docs`. Add a number of extra
dependencies for updating our development tooling including black,
flake8, mypy, and tox.
Add an initial pre-commit pipeline and run across all files in the repo.
Add various fixes for mypy related errors including adding stubs,
ignoring certain dependencies in setup.cfg (for those where stubs
couldn't be located), and manually add type ignore comments where
necessary. These mypy related issues should be revisited in the future
to see if they can be cleaned up in a more coherent way.
!! NOTE !!
Some of these fixes are backwards incompatible changes in API function
and / or argument names.
!! NOTE !!
These changes drop the deprecated "old" database API functions. These
have been marked deprecated since #85 / version 1.1.0 and are multiple
years out of date.
Address linting errors in DMC. Where possible, fix casing issues, add
typing info, and clean up docstrings.

!! NOTE !!
Many of these changes are breaking API changes due to casing adjustments
on the public API. This will require updates in other packages /
modules.
!! NOTE !!
Many of these changes are backwards incompatible API changes due to
casing fixes in the public API.
!! NOTE !!
This includes backwards incompatible changes due to public API changes.
!! NOTE !!
These changes include backwards incompatible public API changes.
Cleanup "missing" function calls due to API changes, minor bugs in
val.py from bad variable name cleanup, and get the test suite running
cleanly again.
Note, this changes the default kwarg zmq_args handling to avoid using a
mutable valuable as a keyword default. The unittest suite runs clean but
this was NOT run in a full data flow use case.
Note, this removes default kwarg zmq_args definitions that use a
mutuable object. This was not tested in a full end to end data flow
test.
Add tox config for building our docs, executing our test suite, and
running our pre-commit linter pipeline. Move all our tests into a top
level tests/ directory to address annoying issues with tox / pytest and
the wrong files being located.

Add MANIFEST.in so our data files get picked up properly in source
distributions.
@MJJoyce MJJoyce requested review from a team as code owners October 26, 2021 16:32
Copy link
Contributor

@jasonmlkang jasonmlkang left a comment

Choose a reason for hiding this comment

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

LGTM!

I notice a lot fo code changes are related to syntax. I guess those are auto updated by pre-commit and related tools. A documentation of those tools would be nice.

@MJJoyce
Copy link
Member Author

MJJoyce commented Oct 27, 2021

LGTM!

I notice a lot fo code changes are related to syntax. I guess those are auto updated by pre-commit and related tools. A documentation of those tools would be nice.

+1 on the docs. Changes are in bound in a PR soon. I have another one coming up with a number of packaging changes and then a bunch of docs updates will follow shortly.

@MJJoyce MJJoyce merged commit 40abda3 into master Oct 27, 2021
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.

3 participants