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

Issue 45 #82

Merged
merged 12 commits into from
Oct 6, 2017
Merged

Issue 45 #82

merged 12 commits into from
Oct 6, 2017

Conversation

Jdsleppy
Copy link
Contributor

@Jdsleppy Jdsleppy commented Oct 5, 2017

Changes:

  • Type annotations to the following (no mypy errors other than missing stubs):
    • course.py
    • lab.py
    • laundry.py
    • library.py
  • Fix lab_test.test_make_status

References:

I was planning on annotating the entire project before making a pull request, but I'm encountering a painful amount of merge conflicts and want to get this in sooner than later. I'm happy to make any changes as needed.

Mypy raised errors when a variable is used to hold two different types. These
were fixed by introduction a new variable instead of reassigning the old one.

The type annotation for get_status_detailed is changed to reflect that
'time_left' is cast to an integer. In get_status_simple it is still returned
as a string, which is inconsistent, but I will leave this decision up to
someone who knows the API better.
Pretty straightforward type annotations, with a couple uses of the vague "Any"
type with complicated dicts.
Make some minimal changes to fix mypy errors. One common type is when a
variable is reassigned.  In another case, mypy wanted a variable to be
explicitly annotated.

I also fixed an error (not sure if an actual funcitonal error, but it was
flagged by mypy) where the & operator was used with two incompatible types.
old_machine_split = status_string.split("\n") #type: List[str]
old_machine_split[0] += machine_name
status_lines = status_string.split("\n") #type: List[str]
status_lines[0] += machine_name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another instance where I normally wouldn't have renamed this variable, but my changes conflicted with someone else who renamed it, and I think the name status_lines is clearer.

from typing import List, Dict, Tuple, Any
from bs4 import BeautifulSoup, SoupStrainer
from typing import Any, Dict, List, Tuple
from bs4 import BeautifulSoup, SoupStrainer, Tag, ResultSet
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebase conflicts... I just tend to put things in alphabetical order.

@@ -19,7 +19,6 @@

import requests
from bs4 import BeautifulSoup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I wouldn't impose my style on the project, but I already had this committed and this stuck around after rebasing...

self.assertEqual(closed[key], 0)
self.assertEqual(open[key], 1)
self.assertEqual(closed[key], '0')
self.assertEqual(open[key], '1')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RitwikGupta
Copy link
Member

This is awesome! I will merge this PR, and then you can continue with this to avoid conflicts :) Thank you!

@RitwikGupta RitwikGupta merged commit a401500 into pittcsc:master Oct 6, 2017
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