-
Notifications
You must be signed in to change notification settings - Fork 172
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] Simplify environment #1133
base: dev
Are you sure you want to change the base?
Conversation
@@ -2,54 +2,43 @@ name: pyjanitor-dev | |||
channels: | |||
- conda-forge | |||
dependencies: | |||
- python=3.9 | |||
- python>=3.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now minimal python version is 3.6
It may don't work in py3.6 with pandas 1.4.1.
it will be updated after setting ci/env
multi-envs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas minimum python version is 3.8, I feel we should make it that, or at least 3.7 because of dictionary order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py3.6 base on setup.py.
As you said, this is the reason I want to build multi-envs
we could test pyjanitor compatible with python3.6 after ci/env/
was built.
Line 124 in 1908f35
python_requires=">=3.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samukweku @Zeroto521 thanks for doing the PR and reviewing!
I think @samukweku's suggestion makes sense here. With pandas
minimum version being 3.8, we should just follow along. I think setup.py
should be updated.
environment-dev.yml
Outdated
- pandas | ||
- pandas-flavor | ||
- multipledispatch | ||
- scipy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scipy
I thought also could as an optional dependency.
there are only two scripts that use scipy
math.py
functions/impute.py
environment-dev.yml
Outdated
- make | ||
# release | ||
- twine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we could drop twine
. we use Github action to build and realse.
environment-dev.yml
Outdated
- pre-commit | ||
- pylint | ||
- isort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could drop pylint
and isort
since we start to use pre-commit.
once we drop these dependencies Makefile also should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having pylint
and isort
binaries in the environment helps with those who use VSCode for code checking. Could we ensure that these stay in the environment while also annotating with comments that they are present for developer convenience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already keep IDE code style checking packages (black, pylint, isort)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also made a PR #1231 making similar changes. I agree with keeping black/isort, for pylint: we don't enforce it via pre-commit, instead we use flake8 as our main linting tool. So I don't see the need for keeping it around.
Codecov Report
@@ Coverage Diff @@
## dev #1133 +/- ##
===========================================
- Coverage 97.34% 41.98% -55.37%
===========================================
Files 77 77
Lines 3240 3244 +4
===========================================
- Hits 3154 1362 -1792
- Misses 86 1882 +1796 |
@samukweku Hi, this problem (test_pivot_longer.py fail) needs you to have a look. |
@Zeroto521 is it possible to extend this to what users can install? I feel it might help with #826 and #793 |
I feel this is triggered because in pandas 3.6, the order of a dictionary is not assured; compared to 3.7 and above, where the order of a dictionary is assured. WIth this, I suggest we make the minimum python version 3.8, as pandas minimum version is 3.8 |
I'm in the progress to do this, see #1127. |
conda will install python from a high version to start
https://github.com/pyjanitor-devs/pyjanitor/runs/7332617267?check_suite_focus=true#step:3:834 |
I finally get some spare time to continue this work. Hope I finished this pr on weekends. |
229c9ee
to
b0605fa
Compare
PR Description
Please describe the changes proposed in the pull request:
functions
this
/environment-dev.yaml
will be kept for the developer to quickly install pyjanitor environment.And
/ci/envs/*
will be created work for CI to test pyjanitor compatibility.This PR resolves #1127.
PR Checklist
Please ensure that you have done the following:
<your_username>
:dev
, but rather from<your_username>
:<feature-branch_name>
.AUTHORS.md
.CHANGELOG.md
under the latest version header (i.e. the one that is "on deck") describing the contribution.Automatic checks
There will be automatic checks run on the PR. These include:
Relevant Reviewers
Please tag maintainers to review.