Implement a XML Review UI that allow user to perform actions on XML files
Review Request #9615 — Created Feb. 10, 2018 and discarded
XML files is handled as an uneditble attachments in the current
version.
I implemented a new classXMLReviewUI
that
enhence Review board's ability to process XML file.Features:
- Smart indentation for the file
- Diff for the file will be shown in render and source formatKnown issues:
- Due to the parser library issue, the XML Render cannot render
the text of an element that are after a child element.
(e.g. <parent>
<child>
</child>
info
</parent> )
info cannot be render in the example above.
Added and ran unit test with:
- Properly formatted XML files (attributes/children).
- Handle Highly-nested XML files.Tested the development environment with:
- File diff system.
- Properly formatted XML files (attributes/children).
- Handle Highly-nested XML files.
Description | From | Last Updated |
---|---|---|
Unlike JSON files, order matters in XML files. For example, imagine this: <h1>This is a section</h1> <p>Some content.</p> <h1>Here's another … |
|
|
These blank lines should remain. Check our style guide on Notion for where blank lines should go. |
|
|
Can you add a comment about why the score is 1 here, based on what we've diagnosed? |
|
|
os is a Python Standard Library module, so it should go in an import group after __future__ but before django. … |
|
|
"XMLReviewUITests" |
|
|
No blank line between class and the docstring. We also tend to include the full module path in the docstring … |
|
|
Helper functions in unit tests should be private (prefix with a _) and put at the end of the class. |
|
|
Typo, and this doesn't follow our documentation guidelines. |
|
|
Ideally, we wouldn't include data files, since that makes it harder to have self-contained tests and to provide variations on … |
|
|
Each component of os.path.join should be a single path entry. No . or /. file_diretory must also be its own … |
|
|
Blank line between these. |
|
|
You want as little as possible in this with block. Confine it to the SimpleUploadedFile creation. |
|
|
Docstrings should be in the form of: """Testing <class>.<method> <condition>""" Like: """Testing XMLReviewUI.get_rendered_lines orders tag results""" Same in other tests. |
|
|
This is all part of the Python Standard Library, so it should be in the same import group. |
|
|
Alphabetical order. This would be: from django.utils import six from django.utils.six.moves import ... from django.utils.translation import ... |
|
|
This is part of the Python Standard Library, and should go in the appropriate import group. |
|
|
Lines are wrapping way too soon. |
|
|
This must describe the results of the method (a Yields section). See Notion for info on writing docs. |
|
|
Blank line between these. |
|
|
We don't use the inline if .. else .. form. This should be expanded into a standard if/else. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Docstrings must have a single-line summary. IT cnanot wrap. Others are wrapping too soon. |
|
|
This is going to show up wrong in the docs. If you want to show inline code, use double backticks … |
|
|
This will need a docstring in our standard format. So will other methods in this class. |
|
|
Double prefixes should never be used in Python, except for operators (like __eq__) or other special methods (__str__, __repr__). Private … |
|
|
This needs a docstring. It's also in the wrong casing format. find_attributes would be more correct. |
|
|
Blank line here. |
|
|
These can be combined into one conditional. |
|
|
Blank line here. |
|
|
This also needs a docstring and should be add_indent. |
|
|
W291 trailing whitespace |
![]() |
|
All your testing happens in the get xml file function, but base on the nasme, that should be returning an … |
RO rodgerwa | |
You don't verify if any exceptions are thrown here. Rather than waiting for a KeyError or TypeError when in etree_to_dict, … |
RO rodgerwa | |
I think some of your variable names (like t, dc and dc) could be more descriptive. For example, I'm assuming … |
JT jtrang | |
This docstring should explain the inputs expected and return values. You also need to say what type is expected for … |
RO rodgerwa | |
Add a comment for why len(value) ==1: is important. |
RO rodgerwa | |
Is it possible for strip() to return none? |
RO rodgerwa | |
You have attrib and attrib_in, both of these side by side are a little confusing. Better names are required. |
RO rodgerwa | |
You should add a comment to explain what is happening here. |
RO rodgerwa | |
Similar comment to above. The variable o could have a more descriptive name. |
JT jtrang | |
Along with providing a better name for the variable, you should add what args and kwargs are representing in your … |
RO rodgerwa | |
Considering this is an IO Stream, is there possibility of not having a value by the end or have a … |
RO rodgerwa | |
You'll need to document the expected results and inputs. |
RO rodgerwa | |
Should say Add indention on new line with an indent of the current depth plus one. |
RO rodgerwa | |
what if the wrong type is passed in? |
RO rodgerwa | |
I understand the _ is for localization? How exactly does that lookup work? |
|
|
Perhaps consider using enums for this and text |
|
|
Not sure this is needed. Why not call self._fixname directly? |
|
|
W291 trailing whitespace |
![]() |
|
It may be more clear if you put key[0] == "text" (or better yet use an enum) and add this … |
|
|
%s may round things (is this what you want?). Consider using %r if you don't want rounding. Have a look … |
|
Change Summary:
I implemented the tag sorting functionality
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+145 -4) |
Checks run (2 succeeded)
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+220 -4) |
Checks run (2 succeeded)
-
-
Unlike JSON files, order matters in XML files. For example, imagine this:
<h1>This is a section</h1> <p>Some content.</p> <h1>Here's another section</h1> <p>More content.</p> <code>Some sample code.</code>
If you order them, then you get:
<code>Some sample code.</code> <h1>This is a section</h1> <h1>Here's another section</h1> <p>Some content.</p> <p>More content.</p>
That doesn't make any sense.
We also shouldn't order attributes. There may be a reason the author has put them that way.
When working with XML, it's good to show the XML in a form that makes it easier to read the document, in a tree form, but we don't want to otherwise alter that content.
-
reviewboard/attachments/mimetypes.py (Diff revision 4) These blank lines should remain. Check our style guide on Notion for where blank lines should go.
-
reviewboard/reviews/ui/base.py (Diff revision 4) Can you add a comment about why the score is 1 here, based on what we've diagnosed?
-
reviewboard/reviews/ui/tests.py (Diff revision 4) os
is a Python Standard Library module, so it should go in an import group after__future__
but beforedjango
. See the style guideline on Notion for import groups. -
-
reviewboard/reviews/ui/tests.py (Diff revision 4) No blank line between
class
and the docstring.We also tend to include the full module path in the docstring for unit tests, so the full one for XMLReviewUI.
The blank line should go after.
-
reviewboard/reviews/ui/tests.py (Diff revision 4) Helper functions in unit tests should be private (prefix with a
_
) and put at the end of the class. -
reviewboard/reviews/ui/tests.py (Diff revision 4) Typo, and this doesn't follow our documentation guidelines.
-
reviewboard/reviews/ui/tests.py (Diff revision 4) Ideally, we wouldn't include data files, since that makes it harder to have self-contained tests and to provide variations on the tests. It's not even needed in this case, since
SimpleUploadedFile
takes the data directly. This function should take the data, and that should be provided in calls to the method instead of opening a file. -
reviewboard/reviews/ui/tests.py (Diff revision 4) Each component of
os.path.join
should be a single path entry. No.
or/
.file_diretory
must also be its own parameter -- no concatenation, since that's the point ofos.path.join
. -
-
reviewboard/reviews/ui/tests.py (Diff revision 4) You want as little as possible in this
with
block. Confine it to theSimpleUploadedFile
creation. -
reviewboard/reviews/ui/tests.py (Diff revision 4) Docstrings should be in the form of:
"""Testing <class>.<method> <condition>"""
Like:
"""Testing XMLReviewUI.get_rendered_lines orders tag results"""
Same in other tests.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) This is all part of the Python Standard Library, so it should be in the same import group.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) Alphabetical order. This would be:
from django.utils import six from django.utils.six.moves import ... from django.utils.translation import ...
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) This is part of the Python Standard Library, and should go in the appropriate import group.
-
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) This must describe the results of the method (a
Yields
section). See Notion for info on writing docs. -
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) We don't use the inline
if .. else ..
form. This should be expanded into a standardif/else
. -
-
-
-
-
-
-
-
-
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) Docstrings must have a single-line summary. IT cnanot wrap.
Others are wrapping too soon.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) This is going to show up wrong in the docs. If you want to show inline code, use double backticks around it.
This format being shown here, though, is a bit confusing, and seems more like an implementation detail. It's better to describe this and then provide an example of a value separately.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) This will need a docstring in our standard format.
So will other methods in this class.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) Double prefixes should never be used in Python, except for operators (like
__eq__
) or other special methods (__str__
,__repr__
).Private methods should also go after all public methods.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) This needs a docstring.
It's also in the wrong casing format.
find_attributes
would be more correct. -
-
-
-
reviewboard/reviews/ui/xmlui.py (Diff revision 4) This also needs a docstring and should be
add_indent
.
Checks run (1 failed, 1 succeeded)
flake8
Description: |
|
---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+233 -2) |
Checks run (2 succeeded)
-
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) I think some of your variable names (like
t
,dc
anddc
) could be more descriptive. For example, I'm assumingdc
means "dictionary children", but it's not immediately obvious. -
reviewboard/reviews/ui/xmlui.py (Diff revision 6) Similar comment to above. The variable
o
could have a more descriptive name.
-
I don't have to much insight on what you are working on but I feel that there are a few things you can do to make this part of the code more maintainable and readable for others.
-
reviewboard/reviews/ui/tests.py (Diff revision 6) All your testing happens in the
get xml file
function, but base on the nasme, that should be returning an xml file of sorts, not testing for nested XML. -
reviewboard/reviews/ui/xmlui.py (Diff revision 6) You don't verify if any exceptions are thrown here. Rather than waiting for a
KeyError
orTypeError
when inetree_to_dict
, you should get anIOError
and handle that. -
reviewboard/reviews/ui/xmlui.py (Diff revision 6) Is this too broad for the types of exceptions beeing handled?
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) flow is really odd and there are no comments to explain what is happening in each block.
You could use intermediate functions to make it obvious too.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) This docstring should explain the inputs expected and return values.
You also need to say what type is expected for
t
because it makes the rest of this code a little unclear as we don't know what attributes are guaranteed to exist. -
reviewboard/reviews/ui/xmlui.py (Diff revision 6) Add a comment for why
len(value) ==1:
is important. -
-
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) Why are you creating a new dict and adding t.tag to it? Is there any benefit over adding text to t.tag?
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) It isn't obvious that this will only be run if
t.attrib
andt.children
areNone
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) confusing operation. You are reassigning an input to the return value of a function to which its intial input was the value you are reassigning.
Maybe use an intermediate?
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) You have
attrib
andattrib_in
, both of these side by side are a little confusing. Better names are required. -
reviewboard/reviews/ui/xmlui.py (Diff revision 6) You should add a comment to explain what is happening here.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) Along with providing a better name for the variable, you should add what
args
andkwargs
are representing in your docstring. -
reviewboard/reviews/ui/xmlui.py (Diff revision 6) Considering this is an IO Stream, is there possibility of not having a value by the end or have a possible IO error?
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) You'll need to document the expected results and inputs.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) More index operations that aren't clear why.
Docstrings could give insight on this.
-
reviewboard/reviews/ui/xmlui.py (Diff revision 6) Should say
Add indention on new line with an indent of the current depth plus one.
-
-
Diff: |
Revision 7 (+250 -2) |
---|
Checks run (1 failed, 1 succeeded)
flake8
-
-
reviewboard/reviews/ui/xmlui.py (Diff revision 7) I understand the _ is for localization? How exactly does that lookup work?
-
-
reviewboard/reviews/ui/xmlui.py (Diff revision 7) Not sure this is needed. Why not call
self._fixname
directly? -
reviewboard/reviews/ui/xmlui.py (Diff revision 7) It may be more clear if you put
key[0] == "text"
(or better yet use an enum) and add this as anand
clause to theif
above it to reduce nesting -
reviewboard/reviews/ui/xmlui.py (Diff revision 7) %s
may round things (is this what you want?). Consider using%r
if you don't want rounding. Have a look at this blog post for some more info.