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.

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

Trim whitespace from uri element text #365

Closed
wants to merge 1 commit into from

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Sep 4, 2020

This copies the whitespace test from #359
but takes a narrower approach by only trimming
whitespace from <uri> element text.

Closes #322.

Signed-off-by: Steve Peters scpeters@openrobotics.org

This copies the whitespace test from gazebosim#359
but takes a narrower approach by only trimming
whitespace from `<uri>` element text.

Closes gazebosim#322.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2020

Codecov Report

Merging #365 into sdf10 will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            sdf10     #365      +/-   ##
==========================================
+ Coverage   87.35%   87.36%   +0.01%     
==========================================
  Files          60       60              
  Lines        9207     9215       +8     
==========================================
+ Hits         8043     8051       +8     
  Misses       1164     1164              
Impacted Files Coverage Δ
src/Material.cc 99.30% <100.00%> (+0.04%) ⬆️
src/parser.cc 78.50% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 805a185...286b459. Read the comment docs.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

only trimming whitespace from element text.

While working on an ign-sensors PR I needed the whitespace fix within a <topic> field: gazebosim/gz-sensors#33 (comment)

I think it's not safe to assume that only URIs need the fix. I wouldn't be surprised to see some of these around, for example:

<name>
    my_robot
</name>

@scpeters scpeters closed this Sep 5, 2020
@scpeters scpeters reopened this Sep 5, 2020
@scpeters
Copy link
Member Author

scpeters commented Sep 5, 2020

clicked the wrong button

@chapulina
Copy link
Contributor

I hear you, but it still feels a bit too heavy-handed to modify every xml file like this

I hear you too, the other PR should affect performance. My main concern at the moment is to keep the behavior backward-compatible.

Maybe there's a less invasive way of doing it. Extending your approach to all string values, maybe? I haven't checked how this affects elements like pose... Does this work, for example?

<pose>
1
2
3
0.1
0.2
0.3
</pose>

@scpeters
Copy link
Member Author

scpeters commented Sep 5, 2020

I hear you, but it still feels a bit too heavy-handed to modify every xml file like this

I hear you too, the other PR should affect performance. My main concern at the moment is to keep the behavior backward-compatible.

Maybe there's a less invasive way of doing it. Extending your approach to all string values, maybe? I haven't checked how this affects elements like pose... Does this work, for example?

<pose>
1
2
3
0.1
0.2
0.3
</pose>

I wrote that but then thought better of it but clicked "close with comment" by accident. I've since deleted it. I'm still thinking it through.

@chapulina chapulina added beta Targeting beta release of upcoming collection 🔮 dome Ignition Dome labels Sep 9, 2020
@scpeters
Copy link
Member Author

scpeters commented Sep 9, 2020

closing in favor of #359

@scpeters scpeters closed this Sep 9, 2020
@scpeters scpeters deleted the uri_squash_whitespace branch September 9, 2020 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants