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

Localize code fixes and analyzers #4100

Merged
merged 4 commits into from
Jul 22, 2024
Merged

Conversation

kant2002
Copy link
Contributor

Closes #1352

@kant2002
Copy link
Contributor Author

I would say this is draft to gather feedback on some key decisions which I have to do.

  • Put constatns in tests
  • Naming convention for resources.
  • Remove IdentifierText from constants, since these value not used.

@StefanOssendorf
Copy link
Contributor

If it's a draft please make it a real draft. Otherwise rocky may just merge it ;)

@rockfordlhotka rockfordlhotka self-requested a review July 15, 2024 18:35
@rockfordlhotka rockfordlhotka marked this pull request as draft July 15, 2024 18:38
@rockfordlhotka
Copy link
Member

My first scan through the code changes is quite positive, I like what you've done so far.

Can you provide more detail about the three decision points, and what you recommend?

@kant2002
Copy link
Contributor Author

@StefanOssendorf unfortunately I press buttons first and think second. So I miss that opportunity 😄

@rockfordlhotka one by one.

  • previously in tests was constants which is used in analyzers. I directly use string literals now. Anyway I cannot leave test code without changes. Maybe i can get to read from prebuilt LocalizationString but for me that’s tying too much to prod code.
  • just voice your opinion on how I name resource strings
  • Just inform that I remove not used code. Maybe it indeed used by some obscure ways.

@kant2002 kant2002 marked this pull request as ready for review July 16, 2024 19:28
@kant2002
Copy link
Contributor Author

@rockfordlhotka can you take a look?

@rockfordlhotka
Copy link
Member

I'm out of town this week, and will look when I get back.

@rockfordlhotka rockfordlhotka merged commit 6640022 into MarimerLLC:main Jul 22, 2024
1 check passed
@kant2002 kant2002 deleted the issue-1352 branch July 22, 2024 20:02
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.

Text in analyzers should be localizable
3 participants