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 default format detection for Databook.load() #488

Closed
wants to merge 1 commit into from

Conversation

nuno-andre
Copy link
Contributor

@nuno-andre nuno-andre commented Dec 13, 2020

Unlike Dataset.load(), which defaults to None, Databook.load() needs an explicit format value.

This code raises TypeError: load() missing 1 required positional argument: 'format':

with open('data.xlsx', 'rb') as fh:
    dbook = Databook().load(fh)

This PR also (without api changes):

  • Adds some tests.
  • Adds some exception messages.
  • Fixes and updates core's docstrings and api docs.
  • Simplifies Databook._package().
  • LBYL → EAFP in .load().
  • Adds an UnsupportedFormat exception for when detect_format fails to avoid the "Format 'None' cannot be loaded.'" message.

@codecov
Copy link

codecov bot commented Dec 13, 2020

Codecov Report

Merging #488 (a875093) into master (7035d79) will increase coverage by 0.60%.
The diff coverage is 89.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   90.67%   91.28%   +0.60%     
==========================================
  Files          28       28              
  Lines        2616     2638      +22     
==========================================
+ Hits         2372     2408      +36     
+ Misses        244      230      -14     
Impacted Files Coverage Δ
src/tablib/core.py 87.05% <76.00%> (+3.08%) ⬆️
tests/test_tablib.py 98.61% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7035d79...a875093. Read the comment docs.

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.

Please do not mix so many issues in one commit. Each issue should be a separate commit, even separate PRs. For max line length, it's still something undecided, I'd rather not change it for now.

@nuno-andre
Copy link
Contributor Author

Not the best PR, indeed. It's a bunch of minor changes prior to push some adapters we're using in prod.

So, in order to ease these and other contributions, would you consider setting those conventions and adding a code formatter as a pre-commit hook? Are there guidelines for this at Jazzband?

@claudep
Copy link
Contributor

claudep commented Dec 29, 2020

Frankly, I think it would be better to let formatting issues aside and concentrate on "real" changes instead, separated by concern.

@nuno-andre
Copy link
Contributor Author

Code formatting reduces diffs and merging conflicts, and saves developers' time. In what sense is not a "real" change?

@hugovk
Copy link
Member

hugovk commented Dec 30, 2020

Thanks for the PR!

Formatting is a real change, but please create another PR or issue for that (or see #401 and linked PRs) and keep this one focused. If anything, it'll help us get this format detection reviewed and merged sooner.

Thank you!

@nuno-andre
Copy link
Contributor Author

Right, I should have searched for such an issue... I'm going to close this PR (it's already too unfocused) and I'll open separate PRs.

Anyway, are there some guidelines for linting and code formatting at Jazzband?

@nuno-andre nuno-andre closed this Dec 30, 2020
@hugovk
Copy link
Member

hugovk commented Dec 30, 2020

It varies by project. This one has some linting done by pre-commit, which you run using the pre-commit commands or with tox -e lint. The CI will check it too.

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.

3 participants