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

Add custom localizer identifiers options #92

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented May 2, 2024

Related to #90

@hishamco
Copy link
Member Author

hishamco commented May 2, 2024

@BrunoJuchli now you can customize the localizer identifiers without enforcing you to use T, 'S', and H which are something special to OC, please have a try and let me know if you have a feedback

@BrunoJuchli
Copy link
Contributor

I've just attempted to test this but now get an exception where previously I didn't. I guess you broke this in commit ff2d977

System.ArgumentOutOfRangeException: startIndex cannot be larger than length of string. (Parameter 'startIndex')
at System.String.ThrowSubstringArgumentOutOfRange(Int32 startIndex, Int32 length)
at System.String.Substring(Int32 startIndex, Int32 length)
at OrchardCoreContrib.PoExtractor.Program.Main(String[] args) in C:\development\OrchardCoreContrib.PoExtractor\src\OrchardCoreContrib.PoExtractor\Program.cs:line 85

In my case

  • inputPath = 'C:\development\smartme-cloud\src\Portal.Web
  • projectFile = C:\development\smartme-cloud\src\Portal.Web\Portal.Web.csproj
  • projectPath = C:\development\smartme-cloud\src\Portal.Web

@hishamco
Copy link
Member Author

hishamco commented May 3, 2024

I will check ASAP

@BrunoJuchli
Copy link
Contributor

I changed the first argument to the path of the solution directory, this works (I do think choosing a project directory previously worked!).

I've also tested passing the argument --localizer S,s and it works as expected 👍

However, I originally understood your comment to mean that you would adapt the tool to search for IStringLocalizer<T> instead of property/field names.
That would make this change redundant.
Is this off the table?

@hishamco
Copy link
Member Author

hishamco commented May 3, 2024

However, I originally understood #90 (comment) to mean that you would adapt the tool to search for IStringLocalizer instead of property/field names.

I think there's a related issue #87 right?

@BrunoJuchli
Copy link
Contributor

BrunoJuchli commented May 24, 2024

However, I originally understood #90 (comment) to mean that you would adapt the tool to search for IStringLocalizer instead of property/field names.

I think there's a related issue #87 right?

No, that would be #90. The other, #87, is unrelated.

To address the problem of detecting incorrect usage, we need logic that understands types, not just names.
If you write code understanding types, I don't see a benefit of it being an analyzer over the PO extraction tool itself. Because you can just write the PO extraction so that it works correctly, no matter the name of the member that IStringLocalizer<T> is assigned to, or the T which is used.
Doing that renders this PR obsolete, because the member name would not matter anymore, at all.
It also means you don't have to add another package, and you could remove some documentation, instead of adding more (documenting the conventions and documenting the command line switch introduced here, would both be obsolete). Overall, it would be less software and less complexity to maintain.

@VincenzoCarlino
Copy link
Contributor

Hi, any update on this?
I really need this feature. Tnx :)

@hishamco
Copy link
Member Author

@VincenzoCarlino I will revise this again today or tomorrow

@hishamco
Copy link
Member Author

@VincenzoCarlino could you please check this one more time

@VincenzoCarlino
Copy link
Contributor

@hishamco I tried locally with a custom localization key ("_localization") it works properly, I got the pot file with all the ids.

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.

3 participants