-
Notifications
You must be signed in to change notification settings - Fork 46
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
Resolve todo fix cp949 to ks_c_5601-1987 #86
Resolve todo fix cp949 to ks_c_5601-1987 #86
Conversation
@HelloWorld017, hello! I decided to get around this way, so that it was more general. |
src/DetectionDetail.cs
Outdated
/// A dictionary for replace unsupported codepage name in .NET to the nearly identical version. | ||
/// </summary> | ||
/// <remarks>ReadOnlyDictionary start support with .NET Framework 4.5</remarks> | ||
private readonly Dictionary<string, string> _fixedToSupportCodepageName = |
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.
Maybe a dictionary is a bit unneeded. What about a if
in the method above?
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.
I would also like to add
Not supported iso-2022-ch? Maybe fix to x-cp50227?
from #78
It can also be expanded in the future.
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.
It's OK if we add those then.
PS I would remove the ReadOnlyDictionary comment. It's a private.
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.
Ready ;)
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.
Ow One other thing , this dictionary could be static? As there is no need to recalculate the values multiple times.
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.
Yes, I didn’t think about it.
cool! nice improvement! Thanks again :) |
Resolve part #78