Skip to content

Conversation

gavishpoddar
Copy link
Contributor

@gavishpoddar gavishpoddar commented Jul 29, 2021

This PR Fixes,

  • Fixing E402 module-level import, not at top of file in dateparser/docs/conf.py
  • Fixing UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 1442: character maps to in Windows

Note: This change makes it possible to run pip install . . But tox is not entirely fixed.

Please suggest.

@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #955 (e36d79b) into master (41f9478) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Copy link
Member

@Gallaecio Gallaecio left a 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.

Comment on lines 24 to 27
# version is used.
sys.path.insert(0, project_root)

import dateparser

Copy link
Collaborator

@noviluni noviluni Aug 7, 2021

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

Copy link
Collaborator

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:

https://github.com/scrapy/parsel/blob/master/docs/conf.py

Copy link
Member

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.

Copy link
Collaborator

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍🏻

Copy link
Collaborator

@noviluni noviluni left a 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
Copy link
Contributor Author

Hi, I have made the changes

pytest.ini Outdated
addopts =
--doctest-modules
--assert=plain
--assert=plain
Copy link

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.

Copy link
Contributor Author

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

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.

4 participants