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

[Design] POStringLocalizer #309

Closed
wants to merge 19 commits into
base: dev
from

Conversation

Projects
None yet
5 participants
@ryanbrandenburg
Member

ryanbrandenburg commented Dec 14, 2016

This is a design PR for the POStringLocalizer work. Missing from the is the pluralization work (other than just to parse the file) because we haven't decided on what the shape of IPluralizedStringLocalizer is.

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Dec 15, 2016

Contributor

Finally we have it 😄 , great job @ryanbrandenburg I appreciate it.
One thing why we didn't finalize the pluralization stuff after that build this on top of that, as we know PO files support pluralization by its nature

Contributor

hishamco commented Dec 15, 2016

Finally we have it 😄 , great job @ryanbrandenburg I appreciate it.
One thing why we didn't finalize the pluralization stuff after that build this on top of that, as we know PO files support pluralization by its nature

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Dec 15, 2016

Contributor

@ryanbrandenburg can you fix the checks failure?

Contributor

hishamco commented Dec 15, 2016

@ryanbrandenburg can you fix the checks failure?

ryanbrandenburg added some commits Dec 15, 2016

}
}
public override Line Parse(string value)

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
public override Line Parse(string value) => null;
@hishamco

hishamco Dec 16, 2016

Contributor
public override Line Parse(string value) => null;
{
public class ObsoleteLine : CommentBase
{
public override string Token

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
public override string Token => "#~ ";
@hishamco

hishamco Dec 16, 2016

Contributor
public override string Token => "#~ ";
{
public class OriginalLine : TokenLine
{
public override string Token

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
public override string Token => "msgid ";
@hishamco

hishamco Dec 16, 2016

Contributor
public override string Token => "msgid ";
}
}
public override Line Parse(string value)

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
public override Line Parse(string value) =>
    new OriginalLine {
        Value = TrimQuotes(TrimToken(new StringBuilder(value))) 
    };
@hishamco

hishamco Dec 16, 2016

Contributor
public override Line Parse(string value) =>
    new OriginalLine {
        Value = TrimQuotes(TrimToken(new StringBuilder(value))) 
    };
{
public class PluralOriginal : OriginalLine
{
public override string Token

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
  public override string Token => "msgid_plural ";
@hishamco

hishamco Dec 16, 2016

Contributor
  public override string Token => "msgid_plural ";
{
public class FlagLine : CommentBase
{
public override string Token

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
public override string Token => "#, ";
@hishamco

hishamco Dec 16, 2016

Contributor
public override string Token => "#, ";
}
}
public override Line Parse(string value)

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
public override Line Parse(string value) =>
    new FlagLine
    {
        Value = TrimToken(new StringBuilder(value)).ToString().Split(',').ToList()
    };
@hishamco

hishamco Dec 16, 2016

Contributor
public override Line Parse(string value) =>
    new FlagLine
    {
        Value = TrimToken(new StringBuilder(value)).ToString().Split(',').ToList()
    };
}
}
private bool IsQuote(char character)

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
private bool IsQuote(char character) =>
    character == '"' || character == '\'';
@hishamco

hishamco Dec 16, 2016

Contributor
private bool IsQuote(char character) =>
    character == '"' || character == '\'';
return character == '"' || character == '\'';
}
private StringBuilder Unescape(StringBuilder value)

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
private StringBuilder Unescape(StringBuilder value) =>
    value.Replace("\\\"", "\"").Replace("\\'", "'");
@hishamco

hishamco Dec 16, 2016

Contributor
private StringBuilder Unescape(StringBuilder value) =>
    value.Replace("\\\"", "\"").Replace("\\'", "'");
{
public class LiteralLine : Line
{
public override Line Parse(string value)

This comment has been minimized.

@hishamco

hishamco Dec 16, 2016

Contributor
public override Line Parse(string value) =>
    new LiteralLine
    {
        Value = TrimQuotes(new StringBuilder(value))
    };
@hishamco

hishamco Dec 16, 2016

Contributor
public override Line Parse(string value) =>
    new LiteralLine
    {
        Value = TrimQuotes(new StringBuilder(value))
    };
@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Dec 16, 2016

Contributor

@ryanbrandenburg It will be nice if POLocalizer implement IPluralizeStringLocalizer that I suggest in PR #305 instead of IStringLocalizer because it doesn't support pluralization, so that's why you need to do it yourself in each implementation that may support pluralization. My aim to eliminate the tedious and repetitive code that we wrote in such cases.

Second thing the amount of code to parse PO is too much, can we simplify this like what we did in INI configuration as example?

Contributor

hishamco commented Dec 16, 2016

@ryanbrandenburg It will be nice if POLocalizer implement IPluralizeStringLocalizer that I suggest in PR #305 instead of IStringLocalizer because it doesn't support pluralization, so that's why you need to do it yourself in each implementation that may support pluralization. My aim to eliminate the tedious and repetitive code that we wrote in such cases.

Second thing the amount of code to parse PO is too much, can we simplify this like what we did in INI configuration as example?

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Apr 5, 2017

Contributor

@sebastienros is that mean that POStringLocalizer will not be supported?!!

Contributor

hishamco commented Apr 5, 2017

@sebastienros is that mean that POStringLocalizer will not be supported?!!

@sebastienros

This comment has been minimized.

Show comment
Hide comment
@sebastienros

sebastienros Apr 5, 2017

Member

Fortunately no. It's just that this branch required more changes so it will be started fresh.

Member

sebastienros commented Apr 5, 2017

Fortunately no. It's just that this branch required more changes so it will be started fresh.

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Apr 5, 2017

Contributor

I see, furthermore I saw that implementation is complicated little bit, It would be nice if we can simplify parsing like what I have seen in INI Configuration

thanks

Contributor

hishamco commented Apr 5, 2017

I see, furthermore I saw that implementation is complicated little bit, It would be nice if we can simplify parsing like what I have seen in INI Configuration

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment