-
Notifications
You must be signed in to change notification settings - Fork 158
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
[#2209] Skip blank lines in blurb file url #2239
Conversation
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.
Thanks for working on this @jedkohjk! Could you update the unit and system tests to include test cases for this scenario?
for (url = ""; url.length() == 0; url = lines.get(position++).strip()) { | ||
// checks if delimiter is the last non-blank line | ||
if (position >= lines.size()) { | ||
return null; | ||
} | ||
} |
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.
Nit: This works well enough, but we can consider using a while
loop, as the linear flow would make it easier to read.
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.
Updated to use a while loop
Modified blurbs files in test resources to account for this scenario |
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.
LGTM! Great work!
@jedkohjk Can you update all your PR titles to include the issue number as per the expected format? |
Updated. Sorry, didn't notice the format previously. |
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.
LGTM
The following links are for previewing this pull request:
|
Resolves #2209
Proposed commit message