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

[INF] Infra upgrades #1294

Merged
merged 30 commits into from
Oct 14, 2023
Merged

[INF] Infra upgrades #1294

merged 30 commits into from
Oct 14, 2023

Conversation

ericmjl
Copy link
Member

@ericmjl ericmjl commented Oct 8, 2023

PR Description

Please describe the changes proposed in the pull request:

  • f773587 test: limit max examples in pytest settings to 10 for multiple test functions
  • 4ee8593 test: limit max examples in pytest settings to 10
  • 3675166 test: limit max examples in pytest settings to 10
  • 0b1b27e Bump urllib3 from 1.26.14 to 1.26.17 in /.requirements (Bump urllib3 from 1.26.14 to 1.26.17 in /.requirements #1292)
  • 6fbd040 refactor(janitor): reorganize function imports and remove unused imports
  • f33a8ef test: import janitor in test_fill_direction.py
  • db953a1 refactor(tests): import janitor module in test files
  • 60ecd75 refactor(janitor/functions): remove unused imports and dynamic import function
  • dfdc31a refactor: update dynamic_import argument and limit test examples
  • 31428fd feat(janitor/functions): add dynamic import functionality
  • 3055c9a feat(utils): add dynamic_import function and import janitor.chemistry in test
  • 41199d4 test: remove redundant dataframe method registration tests
  • 0259c8d refactor(janitor): update import statements and function usage
  • f9a2cbb feat(janitor): add 'col' utility to functions
  • 837c657 chore: remove darglint checks workflow
  • 9f557d4 refactor: update docstrings and remove redundant comments
  • 246badf refactor(janitor/utils): use isinstance for type checking
  • 7da6a9d feat: update pre-commit hooks and add pydoclint
  • c8fd19b chore: disable darglint in pre-commit config
  • 96122f4 infra: satisfy the shiny new ruff linter
  • 0f356e9 chore: Comment out darglint and add --fix arg to ruff
  • 15009f6 chore(environment-dev): update Python and rdkit versions
  • 83f48cb chore(pyproject.toml): Update target Python version to 3.10
  • 7e9253f feat: Add ruff tool configuration to pyproject.toml
  • 40a5f7e chore: update pre-commit hooks

This PR resolves #1236.

PR Checklist

Please ensure that you have done the following:

  1. PR in from a fork off your branch. Do not PR from <your_username>:dev, but rather from <your_username>:<feature-branch_name>.
  1. If you're not on the contributors list, add yourself to AUTHORS.md.
  1. Add a line to CHANGELOG.md under the latest version header (i.e. the one that is "on deck") describing the contribution.
    • Do use some discretion here; if there are multiple PRs that are related, keep them in a single line.

Automatic checks

There will be automatic checks run on the PR. These include:

  • Building a preview of the docs on Netlify
  • Automatically linting the code
  • Making sure the code is documented
  • Making sure that all tests are passed
  • Making sure that code coverage doesn't go down.

Relevant Reviewers

Please tag maintainers to review.

- Removed unused hooks: isort, flake8
- Added new hook: ruff
- Updated black and interrogate hooks configuration
- Commented out darglint due to timeout issues on pre-commit CI

This update aims to improve code quality checks and ensure consistency across the codebase.
This commit introduces a new feature to the codebase by adding the configuration for the ruff tool in the pyproject.toml file.
The configuration includes enabling pycodestyle and Pyflakes codes,
allowing fixes for all enabled rules,
excluding commonly ignored directories,
setting the line length to 88, allowing unused variables when underscore-prefixed,
and assuming Python 3.8.
It also sets the default complexity level to 10 for the mccabe tool under ruff.
This commit updates the target Python version in the pyproject.toml file from 3.8 to 3.10.
- Python version updated from 3.9 to 3.10
- rdkit version constraint removed
In this commit, the darglint pre-commit hook has been commented out. Additionally, the --fix argument has been added to the ruff pre-commit hook.
@ericmjl
Copy link
Member Author

ericmjl commented Oct 8, 2023

Due to performance issues, darglint has been commented out in the pre-commit configuration.
It may be replaced by ruff in the future. See astral-sh/ruff#458 for more details.
- Updated the version of pre-commit-hooks from v4.4.0 to v4.5.0.
- Added pydoclint as an interim replacement for darglint with configuration in pyproject.toml.
Changed the type checking in the skipna function from using type() to isinstance()
for better Pythonic practice.
In this commit, we have updated the docstring for the `_get_data_df` method
in the `DataDescription` class to provide more detailed information about its functionality.
We have also removed the redundant comments from the `__init__` method of the `col` class in `utils.py`
as they were not providing any additional value.
@ericmjl
Copy link
Member Author

ericmjl commented Oct 8, 2023

@thatlittleboy bang bang! 6.2 seconds for pre-commit hooks check 😄

This commit removes the darglint checks workflow from the GitHub actions.
The workflow was initially added to run darglint checks manually due to the pre-commit CI timing out.
Now that the issue has been resolved, the workflow is no longer needed.
@ericmjl ericmjl changed the title Infra upgrades [INF] Infra upgrades Oct 8, 2023
This commit introduces the 'col' utility from the utils module into the janitor package.
This utility can now be accessed directly from the janitor package.
- Updated import statement in __init__.py to include DropLabel from functions.utils
- Modified usage of expand_grid function in expand_grid.py to be directly called instead of through the janitor module
@samukweku
Copy link
Collaborator

Ruff supports darglint? That's fantastic

@ericmjl
Copy link
Member Author

ericmjl commented Oct 8, 2023

Not yet! But there is a replacement for it, pydoclint, which I learned about from the associated issue tracker on Ruff.

@samukweku
Copy link
Collaborator

More power to open source

This commit removes the test_df_registration.py file, which contained redundant tests for dataframe method registration.
These tests were not necessary as the registration of these methods is guaranteed by the pandas-flavor library.
… in test

- Added a new function `dynamic_import` in `janitor/utils.py` that allows for dynamic importing of all modules in a directory.
- Imported `janitor.chemistry` in `tests/chemistry/test_maccs_keys_fingerprint.py` to ensure it's available during testing.
- Also added `importlib` and `pathlib.Path` to `janitor/utils.py` to support the new function.
- Imported dynamic_import from janitor.utils
- Called dynamic_import function with __name__ as argument
- In `janitor/functions/__init__.py`, the argument passed to `dynamic_import` has been updated from `__name__` to `Path(__name__)` to leverage the pathlib library for more robust path handling.
- In `tests/functions/test_conditional_join.py`, the number of examples for several tests has been limited to improve test performance and reduce runtime.
… function

This commit removes the unused imports 'Path' from 'pathlib' and 'dynamic_import' from 'janitor.utils'.
It also removes the call to 'dynamic_import' function which is no longer needed.
- Modified the import statements in test_expand_grid.py and test_factorize_columns.py to include the janitor module.
- This change ensures that the janitor module is explicitly imported in the test files.
This commit adds an import statement for the janitor module in the test_fill_direction.py file.
This is necessary for the proper functioning of the tests in this file.
This commit reorganizes the function imports in the janitor package to improve code readability and maintainability.
It also removes an unused import from the main __init__.py file.
@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

Merging #1294 (a76cf99) into dev (e096587) will decrease coverage by 2.93%.
Report is 2 commits behind head on dev.
The diff coverage is 94.26%.

@@            Coverage Diff             @@
##              dev    #1294      +/-   ##
==========================================
- Coverage   97.20%   94.27%   -2.93%     
==========================================
  Files          78       78              
  Lines        4114     4125      +11     
==========================================
- Hits         3999     3889     -110     
- Misses        115      236     +121     

@ericmjl
Copy link
Member Author

ericmjl commented Oct 8, 2023

@samukweku I think we're good to go here! The patch target is lower, but at least we've got the project coverage up.

ericmjl and others added 4 commits October 8, 2023 14:01
This commit reduces the maximum number of examples generated by pytest for each test case from unlimited to 10.
This change is intended to speed up test execution time without significantly reducing test coverage.
In an effort to optimize testing time,
the maximum number of examples for each test in the pytest settings has been reduced to 10.
This change affects multiple test functions in the 'test_conditional_join.py' file.
@ericmjl
Copy link
Member Author

ericmjl commented Oct 8, 2023

@samukweku I did one other thing to speed up the turtle tests: we only run 10 examples now, rather than the usual 100. That should substantially cut down on the iteration time needed to get feedback on the CI system

@ericmjl
Copy link
Member Author

ericmjl commented Oct 8, 2023

I guess in general, if a Hypothesis-based test takes >1 second to run, we should drop the max number of examples on that test to 10.

ericmjl and others added 5 commits October 9, 2023 06:54
This commit expands the "Write the Code" section in the developer guide.
It provides more detailed instructions on best practices for writing code,
including committing early and often, staying updated with the dev branch, and writing tests.
It also updates the "Check your code" section to include information about pre-commit hooks.
This commit updates the version of the checkout action used in the GitHub Actions workflow from v3 to v4.
It also removes the matrix strategy for running tests, which previously included "turtle" and "not turtle" subsets.
Now, all tests will be run without any subset specification.
This commit introduces a new test for the conditional_join function in the test_conditional_join.py file.
The test uses an example directly from the conditional_join docstring to verify the function's correct operation.
@ericmjl
Copy link
Member Author

ericmjl commented Oct 10, 2023

Okie dokes, everyone, ready for review. Most of the changes don't need to be reviewed, I think (it's just a matter of taking test times down by reducing the number of examples Hypothesis generates), but the others that are infrastructure-related, particularly:

  • darglint -> pydoclint
  • flake8 and others -> ruff

are ready for review.

Tagging @thatlittleboy @Zeroto521 and @samukweku for review please!

@ericmjl ericmjl merged commit a4f1c0a into dev Oct 14, 2023
4 of 6 checks passed
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.

[INF] darglint is archived -> move to ruff?
2 participants