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

fix #524 check for type list and improve error msg #534

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

llazzaro
Copy link
Contributor

Unit test to reproduce the issue #524 and improve error message

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Merging #534 (088513a) into master (bbc2739) will increase coverage by 0.02%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   90.95%   90.97%   +0.02%     
==========================================
  Files          28       28              
  Lines        2652     2660       +8     
==========================================
+ Hits         2412     2420       +8     
  Misses        240      240              
Impacted Files Coverage Δ
src/tablib/core.py 84.07% <75.00%> (+0.11%) ⬆️
tests/test_tablib.py 98.64% <100.00%> (+<0.01%) ⬆️

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

def test_yaml_load(self):
""" test issue 524: invalid format """
yaml_source = Path(__file__).parent / 'files' / 'issue_524.yaml'
with open(str(yaml_source), mode='rb') as fh:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should be able to write with yaml_source.open(mode='rb') as fh:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Contributor

@claudep claudep left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution!

@claudep claudep merged commit 0a3511f into jazzband:master Jan 20, 2023
@llazzaro
Copy link
Contributor Author

Noticed that coverage job failed on master. I will fix it with a new PR.

@claudep
Copy link
Contributor

claudep commented Jan 20, 2023

Frankly, coverage reports are generally so broken I don't even consider them.

@llazzaro
Copy link
Contributor Author

Just in case created this PR https://github.com/jazzband/tablib/pull/535/files

Found a small bug when using empty files. Not sure if it is better to raise an exception or return empty list.

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.

2 participants