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

filter out carriage returns for quasiquotes #113

Merged
merged 2 commits into from
Oct 23, 2013
Merged

filter out carriage returns for quasiquotes #113

merged 2 commits into from
Oct 23, 2013

Conversation

gbwey
Copy link
Contributor

@gbwey gbwey commented Oct 4, 2013

There is a consistency problem when handling carriage returns across Shakespeare and Shelly.
In windows, 'st' returns \r\n but 'textFile' returns \n.

Shelly.readfile returns \r\n but Data.Text.IO.readFile retuns \n. I will add a pull request for Shelly as gregwebs/Shelly.hs#44

(interpolated-string-perl6 'q' and 'qc' return \n only)

I think most consistent behavior is to just drop the '\r' across the board when reading and leave the writing the way it is. This is consistent with Data.Text.IO readFile writeFile.

https://gist.github.com/gbwey/6833805
from the above gist: this is what we have now:
*Test6> main
120 13 10 121 :st
120 10 121 :interpolated string perl6
120 13 13 10 121 :Shelly.writefile st as abc then Shelly.readfile
120 10 10 121 :T.readFile abc
120 10 10 121 :textFile abc

120 13 10 121 :Shelly.readfile ab
120 10 121 :T.readFile ab
120 10 121 :textFile ab

and after the changes for the two pull requests this is the result

*Test6> main
120 10 121 :st
120 10 121 :interpolated string perl6
120 10 121 :Shelly.writefile st as abc then Shelly.readfile
120 10 121 :T.readFile abc
120 10 121 :textFile abc

120 10 121 :Shelly.readfile ab
120 10 121 :T.readFile ab
120 10 121 :textFile ab

Thanks,
Grant

@gregwebs
Copy link
Member

gregwebs commented Oct 4, 2013

awesome, thanks!

The only thing we would change is to only run the filter on the Windows platform so that it doesn't slow down unix.
I am going to ask about this issue on the ghc mail list also.

@gbwey
Copy link
Contributor Author

gbwey commented Oct 4, 2013

It might also be useful on unix if someone ftps a file from windows without
converting the cr/lf, but realistically I am not sure how often that would
happen.
Grant

On Fri, Oct 4, 2013 at 7:19 PM, Greg Weber [email protected] wrote:

awesome, thanks!

The only thing we would change is to only run the filter on the Windows
platform so that it doesn't slow down unix.
I am going to ask about this issue on the ghc mail list also.


Reply to this email directly or view it on GitHubhttps://github.com//pull/113#issuecomment-25736576
.

@gregwebs
Copy link
Member

gregwebs commented Oct 4, 2013

true, we only ever need to strip the carriage return on Windows for the quasi-quote, but we should probably always strip files

@gregwebs
Copy link
Member

gregwebs commented Oct 6, 2013

technically we should probably replace "\r\n" with "\n" or integrate this into our line-breaking function. However I don't see the use case for a lone "\r" in shakespeare.

@gbwey
Copy link
Contributor Author

gbwey commented Oct 6, 2013

I am in favour of eliminating \r as early in the code as possible so no one
ever has to deal with this again. I agree \r doesn't contribute anything
and doesn't have meaning by itself.

On Sun, Oct 6, 2013 at 10:26 AM, Greg Weber [email protected]:

technically we should probably replace "\r\n" with "\n" or integrate this
into our line-breaking function. However I don't see the use case for a
lone "\r" in shakespeare.


Reply to this email directly or view it on GitHubhttps://github.com//pull/113#issuecomment-25768925
.

@gregwebs
Copy link
Member

Thanks, I will add the windows flag. I am too lazy to fix GHC

gregwebs added a commit that referenced this pull request Oct 23, 2013
filter out carriage returns for quasiquotes
@gregwebs gregwebs merged commit 4ed7c89 into yesodweb:master Oct 23, 2013
@gbwey
Copy link
Contributor Author

gbwey commented Oct 23, 2013

That great. Thanks!

On Wed, Oct 23, 2013 at 1:44 PM, Greg Weber [email protected]:

Merged #113 #113.


Reply to this email directly or view it on GitHubhttps://github.com//pull/113
.

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