-
-
Notifications
You must be signed in to change notification settings - Fork 28
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 invalid newline inside inline-table #12
Conversation
@@ -30,18 +30,18 @@ def fmt_project(parsed: TOMLDocument, conf: Config) -> None: | |||
opt_deps = cast(Table, project["optional-dependencies"]) | |||
for value in opt_deps.values(): | |||
normalize_pep508_array(cast(Array, value), conf.indent) | |||
order_keys(opt_deps.value.body, (), sort_key=lambda k: k[0]) | |||
order_keys(opt_deps, (), sort_key=lambda k: k[0]) | |||
|
|||
for of_type in ("scripts", "gui-scripts", "entry-points", "urls"): | |||
if of_type in project: | |||
table = cast(Table, project[of_type]) |
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.
On a completely side-note here, you can also use AbstractTable
in the places you are not sure if you are handling Table or InlineTable.
} | ||
beta = {C = "c",D = "d" | ||
} | ||
alpha = {"A.A" = "a",B = "b"} |
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.
This reads badly, can we add a space between ,
for inline tables? 🤔 while we're at it.
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.
I agree, I was about to ask if that was an stylistic decision, but I suppose it was not 😄
I can give it a try, but currently tomlkit
does not support that. So any solution I propose might sound hacky (I am afraid).
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.
@gaborbernat, I gave it a try in abravalheri#2, but the outcome was not very good.
Indeed, when I solve this problem, I introduce a different one.
I think the best course of action here is to add the sorting capability directly in tomlkit
(or fixing its issue with the reminiscent whitespaces), then proceed doing the change here.
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.
We'll have to follow-up on that then, let's accept this as is, one bugfix at a time.
Signed-off-by: Bernát Gábor <[email protected]>
Hello, this is a possible approach to fix #11.
The approach I took here was to pass the entire table object to the
order_keys
function, so it can be inspected to determine when it is appropriate (or not) to add an empty line.I understand that the changes here might not be exactly what you have in mind, so please feel free to close this PR or ask for changes, I will be happy to implement them.