-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
Alignment alteration #207
base: main
Are you sure you want to change the base?
Alignment alteration #207
Conversation
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## main #207 +/- ##
==========================================
+ Coverage 94.38% 95.00% +0.61%
==========================================
Files 5 5
Lines 2281 2421 +140
==========================================
+ Hits 2153 2300 +147
+ Misses 128 121 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Here's some review suggestions.
src/prettytable/prettytable.py
Outdated
############################## | ||
|
||
|
||
def validate_align(val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these three validators be private/internal and not public?
And let's add type hints.
def validate_align(val): | |
def _validate_align(val: str) -> None: |
src/prettytable/prettytable.py
Outdated
|
||
def validate_align(val): | ||
try: | ||
assert val in ["l", "c", "r"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert val in ["l", "c", "r"] | |
assert val in ("l", "c", "r") |
src/prettytable/prettytable.py
Outdated
raise ValueError(f"Alignment {val} is invalid, use l, c or r") | ||
|
||
|
||
def validate_valign(val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def validate_valign(val): | |
def _validate_valign(val: str | None) -> None: |
src/prettytable/prettytable.py
Outdated
|
||
def validate_valign(val): | ||
try: | ||
assert val in ["t", "m", "b", None] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert val in ["t", "m", "b", None] | |
assert val in ("t", "m", "b", None) |
src/prettytable/prettytable.py
Outdated
raise ValueError(f"Alignment {val} is invalid, use t, m, b or None") | ||
|
||
|
||
def validate_header_align(val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def validate_header_align(val): | |
def _validate_header_align(val: str | None) -> None: |
src/prettytable/prettytable.py
Outdated
# A dict extension to manage field alignment. Used to: | ||
# Have global alingment sepreate from field alignments. | ||
# Validate values when setting for individual fields | ||
class Alignment(dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also private/internal?
class Alignment(dict): | |
class _Alignment(dict): |
src/prettytable/prettytable.py
Outdated
dictrepr = dict.__repr__(self) | ||
return dictrepr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dictrepr = dict.__repr__(self) | |
return dictrepr | |
return dict.__repr__(self) |
Please also add a test case for this method.
src/prettytable/prettytable.py
Outdated
elif field in self._align: | ||
align = self._align[field] | ||
else: | ||
align = self._align.alignment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can you add a test case to cover this line?
tests/test_prettytable.py
Outdated
for field in t.valign: | ||
assert t.valign[field] == "t" | ||
for field in t.header_align: | ||
assert t.header_align[field] == None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint fix:
assert t.header_align[field] == None | |
assert t.header_align[field] is None |
tests/test_prettytable.py
Outdated
t = helper_table() | ||
t.field_names = ["L", "C", "R"] | ||
|
||
assert t.header_align["L"] == None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint fix:
assert t.header_align["L"] == None | |
assert t.header_align["L"] is None |
for more information, see https://pre-commit.ci
…ble into alignment_alteration
Here's a possible solution to a batch of issues with column alignment (#202, #199 , #102, #111). The root of these issues seemed to be the fact that the
align
attribute was filling two roles. It was a global alignment, set as a string, and also a per-column alignment, set as a dictionary. I fixed this by creating a newAlignment
class that is basically a dictionary that contains a variable that stores the global alignment value.The main reason I decided to break it out into it's own single class was that having it as a class allowed me to validate the attributes when the user sets it per column using square brackets. It still functions the same way for the user when they set it, update it, print it, or iterate over it, but it changes the return type to be an instance of
Alignment
rather thandict
.I'd really like to get some comments on this one with some ideas on how I can improve it. I don't love the solution I came up with here, but other things I thought of either would change things from the user's perspective, not address all the issues, or make the code more confusing.