Skip to content

Conversation

spazm
Copy link
Contributor

@spazm spazm commented Oct 25, 2021

Describe your change:

Fixes type annotations on other/date_to_weekday.

  • strptime() returns a datetime, not a datetime.date.
  • dynamic switch from int to str is fine in a dynamic language, but the type checker needs to be told it is expected.

Before

% git checkout master 
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

(% mypy  date_to_weekday.py
date_to_weekday.py:19: error: Incompatible types in assignment (expression has type "str", variable has type "int")
date_to_weekday.py:21: error: Function "datetime.datetime.date" is not valid as a type
date_to_weekday.py:21: note: Perhaps you need "Callable[...]" or a callback protocol?
date_to_weekday.py:22: error: datetime.date? has no attribute "weekday"
Found 3 errors in 1 file (checked 1 source file)

After

% git checkout -
Switched to branch 'mypy-fix-other-date_to_weekday'
Your branch is up to date with 'origin/mypy-fix-other-date_to_weekday'.

% mypy  date_to_weekday.py
Success: no issues found in 1 source file

% mypy  --strict date_to_weekday.py
Success: no issues found in 1 source file
  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@ghost ghost added awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files labels Oct 25, 2021
@spazm spazm force-pushed the mypy-fix-other-date_to_weekday branch from b160d84 to 59a3523 Compare October 25, 2021 08:59
@spazm
Copy link
Contributor Author

spazm commented Oct 25, 2021

apologies: had to rebase my pr branches to fix my author email address.

@@ -14,11 +15,12 @@ def date_to_weekday(inp_date: str) -> str:
>>> date_to_weekday("1/1/2021")
'Friday'
"""
year: Union[int, str]
Copy link
Member

@cclauss cclauss Oct 26, 2021

Choose a reason for hiding this comment

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

Instead of:

from typing import Union
year: Union[int, str]
# this repo has been moving to the new syntax by doing
from __future__ import annotations
year: int | str  # or year: [int | str]

However, I question the base logic of the year % 100 thing because not all years that are evenly divisible by 100 are the same year.

>>> [(i, datetime(i, 1, 1).weekday()) for i in range(200, 2200, 100)]
[ (200, 2),  (300, 0),  (400, 5),  (500, 4),
  (600, 2),  (700, 0),  (800, 5),  (900, 4),
 (1000, 2), (1100, 0), (1200, 5), (1300, 4),
 (1400, 2), (1500, 0), (1600, 5), (1700, 4),
 (1800, 2), (1900, 0), (2000, 5), (2100, 4)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The algorithm was definitely flawed. I just wanted to add types :)

I've rewritten the algo, which boils it down to two calls to the standard lib. If we want to manually parse a string by splitting on /, that can be added back in.

@ghost ghost added the tests are failing Do not merge until tests pass label Oct 27, 2021
>>> day_of_week(datetime(year=2000, month=1, day=1))
'Saturday'
"""
day_of_week: int = datetime_obj.weekday() # Monday = 0 .. Sunday = 6
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep a full implementation here?

Copy link
Member

Choose a reason for hiding this comment

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

My sense is that we should delete the entire file. This is a how-to-use the datetime module, not an algorithm.

Copy link
Member

Choose a reason for hiding this comment

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

@cclauss cclauss added the hacktoberfest-accepted Accepted to be counted towards Hacktoberfest label Oct 29, 2021
@cclauss cclauss changed the title [mypy] Fixes typing errors in other/date_to_weekday Delete other/date_to_weekday.py as a how-to-use, not an algorithm Oct 29, 2021
Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Thanks for your effort @spazm but this file has got to go.

@ghost ghost removed the awaiting reviews This PR is ready to be reviewed label Oct 29, 2021
@cclauss cclauss merged commit a281151 into TheAlgorithms:master Oct 29, 2021
@poyea
Copy link
Member

poyea commented Oct 29, 2021

@spazm In fact the test cases in b9fee14 can go into https://github.com/TheAlgorithms/Python/blob/master/other/doomsday.py. Feel free to open a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This PR modified some existing files hacktoberfest-accepted Accepted to be counted towards Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants