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.

Sign up for GitHub

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 Flake8 #428

Closed
wants to merge 8 commits into from
Closed

Fix Flake8 #428

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9abe323
Format with Black
hugovk Nov 10, 2019
2eaf8c5
Run Black on pre-commit and therefore tox and CI
hugovk Nov 10, 2019
8c2364f
.tox is already a default. Don't overwrite other default excludes
hugovk Nov 10, 2019
fd1922f
Add Black-compatible config to Flake8
hugovk Nov 10, 2019
77c5488
No __cmp__ or cmp in Python 3
hugovk Nov 10, 2019
0987383
Fix Flake8
hugovk Nov 10, 2019
e3b445c
Fix undefined name
hugovk Nov 10, 2019
6bab35b
Run Flake8 on pre-commit and therefore tox and CI
hugovk Nov 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/tablib/formats/_rst.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def export_set_as_simple_table(cls, dataset, column_widths=None):
lines = []
wrapper = TextWrapper()
if column_widths is None:
column_widths = _get_column_widths(dataset, pad_len=2)
column_widths = cls._get_column_widths(dataset, pad_len=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to have a separate PR for this, with a test to complete coverage of that line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could probably remove this whole if block. I don't see how column_widths can be None.

The default value is None in the method definition:

    def export_set_as_simple_table(cls, dataset, column_widths=None):

But it's only called from one place in export_set, and it includes a variable for column_widths, so it won't default to None:

        if use_simple_table and not force_grid:
            return cls.export_set_as_simple_table(dataset, column_widths)
        else:
            return cls.export_set_as_grid_table(dataset, column_widths)

Earlier in that method, it is set as:

        column_widths = cls._get_column_widths(dataset, max_table_width)

Which will always return a list:

        column_widths = [max(w, l) for w, l in zip(column_widths, word_lens)]
        return column_widths

A similar thing happens for the similar if block in export_set_as_grid_table.

These are all in _rst.py, the underscore denoting a private, internal code, and not part of the API.

What do you think?

Copy link
Contributor

@claudep claudep Nov 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure, as the method itself is not prefixed with underscore, so if someone could very well call something like fmt = registry.get_format('rst'); fmt.export_set_as_simple_table(dataset) without breaking private rules. No strong opinion, but I would tend to fix and test.

Copy link
Member Author

Choose a reason for hiding this comment

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

There we go! #433

border = " ".join(["=" * w for w in column_widths])

lines.append(border)
Expand Down