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

Add typing #98

Merged
merged 5 commits into from
Jul 14, 2023
Merged

Add typing #98

merged 5 commits into from
Jul 14, 2023

Conversation

KianKhadempour
Copy link
Contributor

@KianKhadempour KianKhadempour commented Jul 6, 2023

This isn't 100% typed because the libraries that we depend on aren't 100% typed so I had to guess for some things. This caused me to find issue #97 (which you should check out). Apart from typing this PR:

  • Standardizes default/failure values to None instead of some of them being False
  • Uses dt instead of datetime in order to explicitly define timestamp as dt.datetime instead of making the type-hint the module, which was incorrect
  • Changes abstract methods from simply returning to having ...
  • Wraps mess["timestamp_ms"] in int() to explicitly show that it is an int (helpful for type-checking)
  • Adds a few assert statements that make type-checking work
  • Changes all formatted strings to f-strings (to please pylint)

@KianKhadempour
Copy link
Contributor Author

Pytest errors are because | for Union was added in 3.10. I will fix that.

@KianKhadempour
Copy link
Contributor Author

CodeCov patch is just complaining that I formatted the abstract methods "incorrectly" even though they are black defaults because if you do it the way I originally did them (when black complained) they count as covered code.

Copy link
Owner

@joweich joweich 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 your contribution! I added a few early comments

chatminer/chatparsers.py Outdated Show resolved Hide resolved
chatminer/chatparsers.py Outdated Show resolved Hide resolved
chatminer/chatparsers.py Show resolved Hide resolved
chatminer/chatparsers.py Outdated Show resolved Hide resolved
chatminer/chatparsers.py Outdated Show resolved Hide resolved
chatminer/visualizations.py Show resolved Hide resolved
chatminer/visualizations.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2023

Codecov Report

Merging #98 (7ed3e91) into main (b908d2e) will decrease coverage by 0.16%.
The diff coverage is 70.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
- Coverage   71.63%   71.48%   -0.16%     
==========================================
  Files           1        1              
  Lines         275      277       +2     
==========================================
+ Hits          197      198       +1     
- Misses         78       79       +1     
Flag Coverage Δ
unittests 71.48% <70.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
chatminer/chatparsers.py 71.48% <70.00%> (-0.16%) ⬇️

@KianKhadempour
Copy link
Contributor Author

Codecov Report

Merging #98 (289728f) into main (f7f34ff) will decrease coverage by 0.26%.
The diff coverage is 70.73%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
- Coverage   71.63%   71.37%   -0.26%     
==========================================
  Files           1        1              
  Lines         275      276       +1     
==========================================
  Hits          197      197              
- Misses         78       79       +1     

Flag Coverage Δ
unittests 71.37% <70.73%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files Coverage Δ
chatminer/chatparsers.py 71.37% <70.73%> (-0.26%) ⬇️

This is because I added a few variable types.

@joweich
Copy link
Owner

joweich commented Jul 8, 2023

@BlueishTint commented and resolved all open discussions. Is there anything more you want to bring into this PR before merging?

@KianKhadempour
Copy link
Contributor Author

Sorry I was gone for a while with no internet access. I think everything is good here.

@joweich
Copy link
Owner

joweich commented Jul 12, 2023

@BlueishTint fyi, I will fix #99 and merge the fix into this PR. Then I'll merge your PR 👍🏼

@joweich joweich merged commit b8c34bf into joweich:main Jul 14, 2023
9 of 11 checks passed
@joweich joweich mentioned this pull request Aug 31, 2023
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

3 participants