Always create description tag #372
Conversation
|
I'm also not sure how to get that one test to have the correct padding |
wjt
force-pushed
the
always-create-description-tag
branch
from
66477e9
to
693203e
Compare
razzeee
force-pushed
the
always-create-description-tag
branch
from
693203e
to
e0b0b79
Compare
|
I briefly looked at the padding today. It's confusing and I can't quite remember why I felt it to be so important to preserve formatting in badly-formatted files. I think it was because if this automation was to be forced on 500+ apps, I wanted it to be as well-behaved as possible so people didn't hate its PRs. Maybe the logic could be made less fancy and/or that test dropped. But it should be possible to get it to work. I'll try to get back into the mind of myself circa 2019 on a flight home from Greece.. |
I don't know how important the case where the XML has inconsistent indentation is, but previously the result of test_mixed_indentation() was that the closing </release> tag was wrongly outdented. To fix this: - Make the test expect the inserted <description> to follow the prevailing indentation. Previously the test case was expecting it to be indented 2 spaces deeper than its <release>. But in this test case the indentation we are trying to follow is 3 spaces per level. - Make the <description> actually follow the prevailing indentation. Previously we were assuming a 2-space indent when inserting the <description> which led to the </release> element being wrongly indented.
Previously all the test cases, except for one really obnoxious one that uses inconsistent indentation, use a consistent 2-space indent. This ends up masking some bugs in the implementation that incorrectly use a 2-space indent in some cases. Happily it works fine in the common case where the <releases> element already exists with at least one <release> within.
wjt
force-pushed
the
always-create-description-tag
branch
from
e0b0b79
to
efdfa5e
Compare
|
OK, the logic was rewritten since that flight home from Greece, but I kind of got my head around it and figured out something that works. Unfortunately I also discovered that the existing logic is broken in some edge cases, but I marked that new test case as xfail and moved on! |
src/lib/appdata.py
Outdated
|
|
||
| # Indent the opening <description> tag one level | ||
| # deeper than the <release> tag. | ||
| release.text = "\n" + ((releases.text[1::2]) * 3) |
There was a problem hiding this comment.
mypy objects to this because, while I know that releases.text is not None, mypy can't prove it.
Fair enough but it makes the problem harder to fix…
There was a problem hiding this comment.
I guess an if case might help? Pushed an approach.
|
So how to proceed? |
|
@wjt can we move this along? |
|
Sorry, I don't have time to work on this right now. Perhaps you could add an assertion to placate mypy? |
|
I can certainly try, it's only about the test failing? |
| # FIXME: This ends up indenting <releases> correctly, but | ||
| # <release> and <description> incorrectly get the default 2-space |
There was a problem hiding this comment.
Oh I'd forgotten about this one. I think it's such an edge case that we can accept the failure for now.
I think it makes sense to create these empty tags always. Even if it means there will be a few more bytes to download.
I hope this will motivate some more maintainers to actually fill out the description.
Also it will be handy for me, cause I always need to look up the exact syntax whenever I get an update, as I usually forget it.