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

Missing PO Entry when using String Concatenation (+) or Referencing Text defined in const/Field/Property/Variable #87

Open
BrunoJuchli opened this issue Feb 26, 2024 · 8 comments

Comments

@BrunoJuchli
Copy link
Contributor

BrunoJuchli commented Feb 26, 2024

Repro Cases

String Concatenation +

S["Hey there " +
   "how do you like this?" + 
   "I know it's not recommended"];

String Reference (Field/Property...)

const string MyMessage = "This is my message";

....

S[MyMessage];

Actual Behavior

extractpo runs successfully, but the generated *.po file does not contain any entries for these calls.

Expected (Desired) Behavior

Either one of the following would be fine for me:

  • not support these and communicating it when hitting such a case:
    • crash the app with error message indicating the problem
    • exit the app with an error code, and write to
  • support these so that Hey there how do you like this? I know it's not recommended / This is my message turns up in the PO file, respectively.
    • note that the case of referencing an item would require either
      • a) using the S[...] line as context
      • b) or the one of the field/property, but then would require duplicate detection, because the same field/property might be referenced twice!

Note: for the string +-concatenation I would prefer if it would be supported. The reason is, that this is the only supported way how to break a long single-line string into multiple source code lines. All other approaches introduce new lines.
Also note, that the +-concatenation is compiler optimized so that the compilation actually contains a single string -> in the compiled assembly there is no difference between "a" + "b" and "ab"! Something I've learned today :D

@BrunoJuchli BrunoJuchli changed the title Missing PO Entry when using String Concatenation (+) or Referencing Text defined Otherplace Missing PO Entry when using String Concatenation (+) or Referencing Text defined in Field/Property/const Feb 26, 2024
@BrunoJuchli BrunoJuchli changed the title Missing PO Entry when using String Concatenation (+) or Referencing Text defined in Field/Property/const Missing PO Entry when using String Concatenation (+) or Referencing Text defined in const/Field/Property/Variable Feb 26, 2024
@BrunoJuchli
Copy link
Contributor Author

I have reproduced this with the current version on develop - the behavior is the same as the latest release 1.0.1 = the issue still exists.

@hishamco
Copy link
Member

hishamco commented Mar 3, 2024

I will try to reproduce it

@hishamco
Copy link
Member

@BrunoJuchli sorry for the delay on this, could you please write a unit test for each and push a PR for fixing those?

@BrunoJuchli
Copy link
Contributor Author

BrunoJuchli commented Mar 18, 2024

@hishamco
Thanks for getting back to me and the collaboration!
Unfortunately I don't have enough time at the moment to look into this further.
We're still in the decision phase for how we're dealing with translations - if we do actually decide to use this tool I'll probably come back to it.
I'm fine with you closing the issue, if you want to.

It would be really nice if you could release a new version, though. Since this would fix a few of the issues we experience with the tool, it would go a long way to convince the team.

@hishamco
Copy link
Member

We're still in the decision phase for how we're dealing with translations - if we do actually decide to use this tool I'll probably come back to it.

Please let me know if you need any support on the tool, it's my pleasure that the tool is already used by OC and hopefully other projects

It would be really nice if you could release a new version, though. Since this would fix a few of the issues we experience with the tool, it would go a long way to convince the team.

Ya I will do ASAP

@BrunoJuchli
Copy link
Contributor Author

I've figured out the code to extract the string from a string literal concatenation expression like:

["foo" + "bar" + "licious"]

here's the code:

public static bool IsStringLiteralExpression(
        ExpressionSyntax expression,
        [NotNullWhen(true)] out string? stringLiteralExpression)
    {
        if(GetLiteralExpressionValue(expression) is { } s)
        {
            stringLiteralExpression = s;
            return true;
        }

        string concatenatedRights = string.Empty;
        while(expression is BinaryExpressionSyntax binaryExpressionSyntax)
        {
            if (GetLiteralExpressionValue(binaryExpressionSyntax.Right) is not { } right)
            {
                break;
            }

            concatenatedRights = right + concatenatedRights;
            
            if (GetLiteralExpressionValue(binaryExpressionSyntax.Left) is { } left)
            {
                stringLiteralExpression = left + concatenatedRights;
                return true;
            }

            expression = binaryExpressionSyntax.Left;
        }

        stringLiteralExpression = null;
        return false;
    }

    static string? GetLiteralExpressionValue(
        ExpressionSyntax expression)
    {
        if(expression is LiteralExpressionSyntax literal &&
           literal.IsKind(SyntaxKind.StringLiteralExpression))
        {
            return literal.Token.ValueText;
        }

        return null;
    }

I've tested this with 2, 5 and 6 concatenations.

I'm not planning on creating a PR since we're not going to use POExtractor after all.
I'm writing our own tool, extracting to JSON (and using type detection instead detecting by variable name, and so far it works good, it's just a bit slower).

@hishamco
Copy link
Member

I'm writing our own tool, extracting to JSON (and using type detection instead detecting by variable name, and so far it works good, it's just a bit slower).

As I said before you can skip the limitation of the variable names, check #92

@BrunoJuchli
Copy link
Contributor Author

I'm writing our own tool, extracting to JSON (and using type detection instead detecting by variable name, and so far it works good, it's just a bit slower).

As I said before you can skip the limitation of the variable names, check #92

yes, thanks, but as I said before, I don't like it - because it actually doesn't eliminate the limitation, it just broadens the list of possible values.
And again, if you ever write an analyzer for this, you can only analyze it if you check the types, not just the name, and you just end up with an unnecessarily complicated solution:
checking names and types - where checking the types would suffice.

Also, my solution respects the <T> parameter, another considerable advantage - in my opinion. And again something you can only achieve by dealing with types.
Of course there's two downsides to my solution as well:

  • only works when the solution can be compiled
  • takes longer

In my environment I anticipate these downsides as having minimal impact.


Feel free to close the issues I've reported.

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

No branches or pull requests

2 participants