Skip to content

preserve encoding over temporary storage - #482

Merged
heejaechang merged 4 commits into
dotnet:masterfrom
heejaechang:preserveEncoding
Feb 14, 2015
Merged

preserve encoding over temporary storage#482
heejaechang merged 4 commits into
dotnet:masterfrom
heejaechang:preserveEncoding

Conversation

@heejaechang

Copy link
Copy Markdown
Contributor

little bit of refactoring over temporary stroage and text factory service to preserve encoding round trip

this fixes #380 and #353

little bit of refactoring over temporary stroage and text factory service to preserve encoding round trip

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add another test (or enhance this one) to test non-null encodings too.

foreach(var encoding in new Encoding[] {
    null,
    Encoding.ASCII,
    Encoding.UTF8,
    Encoding.Unicode,
    Encoding.BigEndianUnicode})
{
    var text = Text.SourceText.From("Hello, World!", encoding);
    // etc.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, please do that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@pharring

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment
@tmeschter

Copy link
Copy Markdown
Contributor

LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you comment this function? I find it confusing that an Encoding is passed in, but then the code calls "detectEncodingFromBytOrderMarks" and then calls AsRoslynText, not using the encoding passed in, but instead reader.CurrentEncoding or Encoding.UTF8.

It seems like someone can be explicit about the encoding, but still have that overridden by the system.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

default encoding is something it use if it can't figure out Encoding from stream. that is why it is called default encoding. I will add more comments.

But I agree it is confusing. I took me sometime to figure out what that actually mean.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

New API I added. CreateText(TextReader reader ... ) is the API people should use if they want to set encoding explicitly. this API doesnt have ambiguity where given Encoding and encoding embedded in the stream is different.

the first API is intended to be used in a situation where you just open a file (FileStream) and want to create a SourceText. the default encoding given is basically saying, if you can't figure out Encoding from the stream I gave you, assume it is the default encoding.

heejaechang pushed a commit that referenced this pull request Feb 14, 2015
preserve encoding over temporary storage
@heejaechang
heejaechang merged commit 274b5c8 into dotnet:master Feb 14, 2015
dibarbet pushed a commit that referenced this pull request Aug 18, 2026
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.

TetsTemporaryStorageText failing with null Encoding

7 participants