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

Warn datetime.{utcnow,utcfromtimestamp} usages #259

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

sobolevn
Copy link
Contributor

@sobolevn sobolevn commented Jul 16, 2023

Closes #258

Things I did:

  1. The warnings itself
  2. Docs
  3. Tests
  4. Plus, I added tmp.py to .gitignore, because it is advertised to be used in adding-new-checks

Open questions:

  • I find it quite unusual to install a dev package with pip install . and not pip install -e . Is it intentional?
  • Why do you use dev-requirements.txt with poetry?

We can discuss these questions in separate issues if you wish :)

Anyways, thanks a lot for the great package!

Copy link
Owner

@dosisod dosisod 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 for opening this PR! I'm a bit busy atm so I'm leaving a partial review, though I'll make sure to give this a full review by this time tomorrow.

test/data/err_176.txt Outdated Show resolved Hide resolved
@dosisod
Copy link
Owner

dosisod commented Jul 17, 2023

Also, to answer your open questions:

  • pip install . is a mistake, it should be pip install -e .. It's mainly used for CI (since it doesn't need to edit anything), but it doesn't hurt to make it editable, and is confusing for users if they run make install locally.
  • I don't use Poetry, but I generated the pyproject.toml file using Poetry, so it automatically pulled in my dev dependencies. Those deps aren't kept up-to date since CI uses dev-requirements.txt. That leaves me with 2 options:
    • Delete the tool.poetry.dev-dependencies block (not ideal for Poetry users)
    • Keep the dev dependencies in sync with the dev-requirements.txt (tedious)

Personally I prefer to pin my dev dependencies, but make sure they are regularly updated. I could probably do the same thing in Poetry, though I don't know how to do dev dependency pinning in Poetry (I'll look it up later).

I'm open to solutions for handling dependencies better, so if you have any ideas on that I'd like to hear it!

sobolevn added a commit to sobolevn/refurb that referenced this pull request Jul 17, 2023
So devs will have their code updated automatically without the need to run `make install` each time.

See dosisod#259 (comment)
Copy link
Owner

@dosisod dosisod left a comment

Choose a reason for hiding this comment

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

Overall this looks really good! I left a few of minor notes, but once those are addressed I'll go ahead and merge this in!

refurb/checks/datetime/unreliable_utc_usage.py Outdated Show resolved Hide resolved
refurb/checks/datetime/unreliable_utc_usage.py Outdated Show resolved Hide resolved
refurb/checks/datetime/unreliable_utc_usage.py Outdated Show resolved Hide resolved
test/data/err_176.py Show resolved Hide resolved
Copy link
Owner

@dosisod dosisod left a comment

Choose a reason for hiding this comment

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

Looks good to me! Sorry about CI failing, isort should be disabled for the test data folder. I'll fix that tomorrow.

@dosisod dosisod merged commit a243a57 into dosisod:master Jul 18, 2023
2 checks passed
dosisod pushed a commit that referenced this pull request Jul 27, 2023
So devs will have their code updated automatically without the need to run `make install` each time.

See #259 (comment)
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.

[Enhancement]: Warn datetime.utcnow and datetime.utcfromtimestamp usages
2 participants