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

Remove unneeded content_type strip #642

Merged
merged 1 commit into from
Jan 13, 2023
Merged

Conversation

albertyw
Copy link
Member

Based on the comment #641 (review), there's not a good reason to have these lines that attempt to strip content_type.

If strip() is a method on a string, strip() will return a string making the strip() a few lines below superfluous (s.strip().strip() == s.strip()). If strip() does trigger the try/catch, the try/catch will be ineffective anyways because the later strip() will also trigger an AttributeError. There is a possibility that a different type implements strip() in a way that multiple strips have different behavior, but it doesn't seem possible given how the _parse_content_type function is called.

@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #642 (3bd3de9) into master (602921e) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
+ Coverage   86.33%   86.40%   +0.06%     
==========================================
  Files          52       52              
  Lines        2093     2089       -4     
==========================================
- Hits         1807     1805       -2     
+ Misses        286      284       -2     
Impacted Files Coverage Δ
silk/model_factory.py 84.41% <ø> (+0.58%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@albertyw albertyw merged commit 4784e2a into jazzband:master Jan 13, 2023
@albertyw albertyw deleted the remove-strip branch January 13, 2023 05:33
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