-
Notifications
You must be signed in to change notification settings - Fork 489
Fixing: flake8 E402 and windows setup.py not working#955
Fixing: flake8 E402 and windows setup.py not working #955gavishpoddar wants to merge 10 commits intoscrapinghub:master from
Conversation
...position 1442: character maps to <undefined> in Windows
Codecov Report
Merging #955 (e36d79b) into master (41f9478) will not change coverage.
The diff coverage isn/a.
@@ Coverage Diff @@ ## master #955 +/- ## ======================================= Coverage 98.29% 98.29% ======================================= Files 234 234 Lines 2694 2694 ======================================= Hits 2648 2648 Misses 46 46
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 41f9478...e36d79b. Read the comment docs.
@Gallaecio
Gallaecio
left a comment
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.
The fixes seem safe to me.
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.
From what I read in the comments:
Insert the project root dir as the first element in the PYTHONPATH.
This lets us ensure that the source package is imported, and that its
version is used.
I think that this import is here intentionally. I'm not sure if moving it from here would have any adversarial consequence. Wouldn't be better to change it to...
import dateparser # noqa: E402
just in case? @Gallaecio
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.
BTW, I've seen that in parsel has the import above, so maybe I'm worrying unnecessarily:
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 had not thought of that. Indeed, having it after will prevent a system-wide-installed version of dateparser being used instead.
It should not be a problem when using tox though, I believe, since tox installs the project into its virtual environment every time.
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 prefer to keep it where it was to avoid needing to modify the comments, etc.
@gavishpoddar could you move it again where it was and add the "noqa"?:
import dateparser # noqa: E402
Thanks!
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.
Done 👍🏻
@noviluni
noviluni
left a comment
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.
Could you remove the dateparser/docs/conf.py E402 line from the pytest.ini?
thanks!
gavishpoddar
commented
Aug 7, 2021
Hi, I have made the changes
pytest.ini
Outdated
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.
It shows "No newline at the end of file" so we need to add it.
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.
Hi @kishan3, Currently dateparser doesn't have newlines at the end .ini files please suggest should I add them with this PR
Note: The tests are not failing without newline at the end of file
Uh oh!
There was an error while loading. Please reload this page.
This PR Fixes,
dateparser/docs/conf.pyNote: This change makes it possible to run
pip install .. But tox is not entirely fixed.Please suggest.