Add regular expression (regex) support to LineText2 protocol's delimiter #706
Conversation
| def set_delimiter delim | ||
| @lt2_delimiter = delim.to_s | ||
| if delim.class == Regexp |
There was a problem hiding this comment.
The case block may be cleaner:
@lt2_delimiter = case delim
when Regexp
delim
else
delim.to_s
end
|
Thanks for the recommendation. I modified the commit with your code recommendation. |
|
Somehow I captured thedrow's commit in this pull request. Let me know and I will delete this pull request and submit a new one. |
|
Since you're submitting your commit from your master branch - let's assume that you have a remote called upstream - try this: $ git checkout master
$ git fetch upstream
$ git rebase upstream/master
$ git push -f origin masterTo check on your remotes (this is how mine look): $ git remote -v
origin git@github.com:sodabrew/eventmachine (fetch)
origin git@github.com:sodabrew/eventmachine (push)
upstream git@github.com:eventmachine/eventmachine.git (fetch)
upstream git@github.com:eventmachine/eventmachine.git (push)To add a remote: $ git remote add upstream git@github.com:eventmachine/eventmachine.git |
|
You also do not need to submit new pull requests. A PR matches with a source/dest branch. You can add and remove commits on your branch and the PR will update to reflect whatever is on your branch. |
|
I can try reverting and re-commiting. I am not sure what to do here. Unfortunately, I do not have much experience with pull requests. |
|
I think the cleanest solution may be to close this pull request and submit a new one. Thoughts? |
|
I supplied the necessary commands above, happy to help explain further if it's not working. |
|
Now that you've added a revert commit that we do not want to have on this branch, you'll need to do an interactive rebase: $ git rebase -i upstream/masterWhen you're inside the editor window, delete the line with the README commit and delete the line with your Revert README commit. Leave only the line with your Line Protocol commit. Then save and exit and let git do the work of adjusting the commits on your local branch. Force-push your adjusted branch to GitHub: $ git push -f origin master |
|
I reset back to before the revert commits. I think it is fixed now. When you merge, you may want to make sure that README.md is not negatively affected. I did not modify it, but it showed modified on my forked master branch when I reset back to my original commit. This appears to have worked and thank you for your help. I use git everyday, but I learned something new today. |
|
You got it! Well done, let's let the tests execute... |
| @@ -57,6 +57,7 @@ def receive_data data | |||
| while remaining_data.length > 0 | |||
| if @lt2_mode == :lines | |||
| if ix = remaining_data.index( @lt2_delimiter ) | |||
There was a problem hiding this comment.
Does String#index behave correctly with a Regex argument?
There was a problem hiding this comment.
It does according to ruby's documentation and the test.
|
I have refactored the code. The new code is not a elegant to read, but I believe it will be much more efficient in execution. The only additional computation cost for the string case is a handful of extra conditional operations and some string assignments. There are no extra searches required. In the regular expression case, the Regex is only evaluated once and further searches are done with strings which will be much more efficient. This re-factor has many more lines of code, but I think the improvement in execution speed is worth it. Note: I have not done any benchmark testings, but I assume string searches are more expensive than conditionals and string searches. Please let me know if you have any questions. |
| @lt2_linebuffer << remaining_data[0...ix] | ||
| ln = @lt2_linebuffer.join | ||
| @lt2_linebuffer.clear | ||
| if @lt2_delimiter == "\n" | ||
| ln.chomp! | ||
| end | ||
| receive_line ln | ||
| remaining_data = remaining_data[(ix+@lt2_delimiter.length)..-1] | ||
| remaining_data = remaining_data[ix+delimiter_string.length..-1] |
There was a problem hiding this comment.
Suggest adding back the parens for clarity.
…ter. The set_delimiter method will accept a regular expression and set the end-of-line delimiter appropriately. Anything passed to the set_delimiter method other than a regular expression will be converted to a string.
|
Thanks for the suggestions. The pull request has been updated. |
This mod allows the use of a regular expression as the end-of-line delimiter in the LineText2 protocol. The set_delimiter method will accept a regular expression and set the end-of-line delimiter appropriately. Anything passed to the set_delimiter method other than a regular expression will be converted to a string. A new test to capture the use of a regular express was added.