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

🐛 Use window of correct size in presence of newlines to grab content from xml files #461

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

pranavgaikwad
Copy link
Contributor

@pranavgaikwad pranavgaikwad commented Jan 8, 2024

We are stripping out empty lines when doing the search, however, in the window that we load into memory we still have to load these empty lines to be able to correctly search content. We only have to get rid of newlines from beginning and the end.

@pranavgaikwad pranavgaikwad changed the title 🐛 Use window of correct size to grab content from xml files 🐛 Use window of correct size in presence of newlines to grab content from xml files Jan 8, 2024
lines = append(lines, line)
}
// remove leading and trailing empty lines
if len(lines) > 0 && lines[0] == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a time/place where you can have content that is 5 empty lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawn-hurley content comes from innerText returned by the XML search, and it depends on the xml query, so theoritically it's possible if someone is intentionally / mistakenly looking for empty lines. in that case, we will end up showing empty lines as code snip

Copy link
Contributor

Choose a reason for hiding this comment

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

What if as we loop, as long as one thing has content, then we use it, if all don't then we skip it?

if len(lines) > 0 && lines[len(lines)-1] == "" {
lines = lines[:len(lines)-1]
}
if len(lines) < 1 || strings.Join(lines, "") == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawn-hurley this check should take care of the empty case

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@pranavgaikwad pranavgaikwad added the cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch. label Jan 9, 2024
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

Looks good to me

@pranavgaikwad pranavgaikwad merged commit a9927b9 into konveyor:main Jan 9, 2024
6 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 9, 2024
…from xml files (#461)

We are stripping out empty lines when doing the search, however, in the
window that we load into memory we still have to load these empty lines
to be able to correctly search content. We only have to get rid of
newlines from beginning and the end.

---------

Signed-off-by: Pranav Gaikwad <[email protected]>
pranavgaikwad added a commit that referenced this pull request Jan 9, 2024
…from xml files (#461) (#462)

We are stripping out empty lines when doing the search, however, in the
window that we load into memory we still have to load these empty lines
to be able to correctly search content. We only have to get rid of
newlines from beginning and the end.

---------

Signed-off-by: Pranav Gaikwad <[email protected]>

Signed-off-by: Pranav Gaikwad <[email protected]>
Co-authored-by: Pranav Gaikwad <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants