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

Lint drift detection part #262

Merged

Conversation

garawalid
Copy link
Collaborator

Part of #219

@smastelini
Copy link
Contributor

Hi @garawalid. Thank you for keeping the linting efforts! I left a small comment that might address the cause why the tests are failing.

@garawalid
Copy link
Collaborator Author

garawalid commented Jul 2, 2020

@smastelini thanks for the review! I need also to fix some regression in the lint of src/skmultiflow/data

@codecov
Copy link

codecov bot commented Jul 2, 2020

Codecov Report

Merging #262 into master will increase coverage by 0.02%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   88.39%   88.42%   +0.02%     
==========================================
  Files         185      185              
  Lines       13566    13571       +5     
==========================================
+ Hits        11992    12000       +8     
+ Misses       1574     1571       -3     
Impacted Files Coverage Δ
...skmultiflow/drift_detection/base_drift_detector.py 86.95% <ø> (ø)
src/skmultiflow/drift_detection/page_hinkley.py 100.00% <ø> (ø)
src/skmultiflow/drift_detection/hddm_w.py 94.73% <90.00%> (+0.05%) ⬆️
src/skmultiflow/data/time_manager.py 89.79% <100.00%> (ø)
src/skmultiflow/drift_detection/adwin.py 92.59% <100.00%> (ø)
src/skmultiflow/drift_detection/ddm.py 100.00% <100.00%> (ø)
src/skmultiflow/drift_detection/eddm.py 100.00% <100.00%> (ø)
src/skmultiflow/drift_detection/hddm_a.py 100.00% <100.00%> (ø)
src/skmultiflow/drift_detection/kswin.py 100.00% <100.00%> (ø)
... and 1 more

@garawalid
Copy link
Collaborator Author

Something wrong with flake8 and src/skmultiflow/data. In fact, the pipeline passes in the last commit in the master (Also on my local machine). And I didn't change it here.

@garawalid
Copy link
Collaborator Author

In fact, there is a bug in flake8 (pycodestyle) with W605 and W503. I reported it here PyCQA/pycodestyle#949. I'll disable both rules for instance!

@garawalid garawalid self-assigned this Jul 2, 2020
@garawalid garawalid marked this pull request as ready for review July 2, 2020 22:58
@garawalid
Copy link
Collaborator Author

It was a mistake in the tox.ini file and I fixed it! This PR is ready for review.

@smastelini
Copy link
Contributor

Thank you for the hard work @garawalid. I'll proceed to review the PR.

@smastelini smastelini self-requested a review July 6, 2020 13:56
Copy link
Contributor

@smastelini smastelini left a comment

Choose a reason for hiding this comment

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

Thank you again @garawalid for the linting efforts. I finished a first pass through the changes and let some comments and questions. It's looking pretty good so far!

@garawalid
Copy link
Collaborator Author

Thanks @smastelini for the review. I think your tox.ini uses ignore instead of extend-ignore.
The W503 is disabled by default. See the full list here and here.

@smastelini
Copy link
Contributor

Thanks @smastelini for the review. I think your tox.ini uses ignore instead of extend-ignore.
The W503 is disabled by default. See the full list here and here.

Thanks for the explanation and for pointing me out to these guidelines, @garawalid. In fact, I found these W503 violations visually :-)

Although W503 is ignored by default, (and only for readability and aesthetic purposes) I personally would prefer to enforce this rule. But it is worth discussing further. Any thoughts on that matter, @jacobmontiel?

@garawalid
Copy link
Collaborator Author

@smastelini
The W503 will be an anti-pattern, that's why they introduced the W504 (the opposite).

Note: Despite being in the anti-pattern section, this will soon be considered the best practice.

I ve been reading some comments about W503 & W504. And it sounds that the community didn't reach a final agreement yet.
I'll vote to disable them both (the default configuration of flake8).
Correct style for line breaks when chaining methods in Python

@smastelini
Copy link
Contributor

smastelini commented Jul 10, 2020

Hi @garawalid, you convinced me hahaha.

I would say, let's go for W504 (even if we do not enforce it in our checks). What do you think @jacobmontiel?

@garawalid
Copy link
Collaborator Author

@smastelini sorry for the late response, I missed some comments.
Could you please take another look whenever you have some free time?

@smastelini
Copy link
Contributor

Hi @garawalid, thanks for the changes. I left some additional comments and resolved some of the pending items that were already solved.

@smastelini
Copy link
Contributor

Thanks Walid for your linting efforts! This PR is now ready to be merged.

@smastelini smastelini merged commit db0e7fb into scikit-multiflow:master Jul 24, 2020
@garawalid
Copy link
Collaborator Author

Thanks @smastelini for your time and the review!

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.

None yet

2 participants