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

parsers/base: make sure content always ends with an empty line #130

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

XaF
Copy link

@XaF XaF commented Nov 7, 2019

For some whois output where the content does not end with an empty new line, the whois call was returning an Unexpected token: % (c) 2019 Canadian Internet Registration Authority, (http://www.cira.ca/) error, for instance.

This fixes that by forcing into existence that ending new line.

Note that this is going to be followed by another PR to support the last CIRA whois output, which is currently failing.

For some whois output where the content does not end with an
empty new line, the whois call was returning an 'Unexpected token:
% (c) 2019 Canadian Internet Registration Authority,
(http://www.cira.ca/)' error, for instance.

This fixes that by forcing into existence that ending new line.
@joallard
Copy link

Looking at the build, looks like this is Ruby 2.5+ only because of String#delete_suffix.

@XaF
Copy link
Author

XaF commented Feb 21, 2020

Hah! Trying with gsub to fix that then :)

@XaF
Copy link
Author

XaF commented Feb 21, 2020

Seems there are issues with other registrars now... Unrelated to what I changed

@joallard
Copy link

@XaF I guess that as long as it doesn't introduce new test breaks, it's mergeable.

(Interesting fixup technique by the way, I figure you show your fixups but offer squashing that right before the merge?)

@@ -363,7 +363,7 @@ def response_unavailable?
protected

def content_for_scanner
@content_for_scanner ||= content.to_s.gsub(/\r\n/, "\n")
@content_for_scanner ||= "#{content.to_s.gsub(/\r\n/, "\n").gsub(/\n$/, '')}\n"

Choose a reason for hiding this comment

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

As a matter of readability, it's not immidiately clear what we're doing here and why we're doing it.

Copy link
Author

@XaF XaF Jul 14, 2020

Choose a reason for hiding this comment

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

What change would you suggest ?
Would that be acceptable ?

Suggested change
@content_for_scanner ||= "#{content.to_s.gsub(/\r\n/, "\n").gsub(/\n$/, '')}\n"
@content_for_scanner ||= begin
content_str = content.to_s
content_str.gsub!(/\r\n/, "\n")
# We need a new line at the end, which exists in some cases, and does
# not in others, we will thus remove the ending new line character if it
# exists (or do nothing if it does not), then return the content appended
# with a new line character
content_str.gsub!(/\n$/, '')
"#{content_str}\n"
end

it "returns the part body with line feed normalized" do
instance = described_class.new(Whois::Record::Part.new(:body => "This is\r\nthe response.", :host => "whois.example.test"))
expect(instance.send(:content_for_scanner)).to eq("This is\nthe response.")
expect(instance.send(:content_for_scanner)).to eq("This is\nthe response.\n")

Choose a reason for hiding this comment

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

So I understand that it's better for the scanner to ingest whole lines (including linebreak) instead of just the string on it, thus rendering the line empty but still present, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes

@XaF
Copy link
Author

XaF commented Jul 14, 2020

@XaF I guess that as long as it doesn't introduce new test breaks, it's mergeable.

(Interesting fixup technique by the way, I figure you show your fixups but offer squashing that right before the merge?)

Sorry for the delay in getting back to you for this @joallard (wow, and what a long delay...)
I actually have a git command to fast create the fixup. Locally I generally do a git rebase -i --autosquash once everything is ready (and git push -f in this case). Adding new fixup commits instead of force-pushing during development allow reviewers on github to see what they've not seen yet instead of restarting the review from scratch. Another possibility for the squash is that the person merging the commit in GitHub uses the Squash and merge option, which does the autosquash automatically (that person would then have to remove in the description all the separate commits though, and keep only the main one, since all the fixup! are not really interesting to see in the git log)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants