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

Change the code using string operations e.g IndexOf and decide the correct way to call it #1862

Open
2 of 6 tasks
tarekgh opened this issue Sep 14, 2022 · 2 comments
Open
2 of 6 tasks
Labels

Comments

@tarekgh
Copy link

tarekgh commented Sep 14, 2022

Read and complete the full issue template

Do not randomly delete sections. They are here for a reason.

Do you want to request a feature or report a bug?

  • Bug
  • Feature
  • Question

Did you test against the latest CI build?

  • Yes
  • No

If you answered No, please test with the latest development build first.

Version of ClosedXML

latest.

What is the current behavior?

The code is using some string operation like IndexOf without passing the StringComparison option. by default doing that the operation will assume want linguistic behavior. This can produce some surprises and wrong results in some situations. for example

if (color.IndexOf("[") >= 0)
search for "[" inside the string. this operation can succeed in some cases even the source string not including [ character. Like if the user current culture is set to Thai culture. The fix for such case can be either do the search using a character instead of string like IndexOf('[') or passing StringComparison parameter like IndexOf("[", StringComparison.Ordinal)

Maybe will be a good idea enable the analyzers listed in https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/globalization-warnings?view=vs-2022 on your codebase.

What is the expected behavior or new feature?

The string operations in the code should always produce the desired result and be protected from surprising behaviors.

Complete this.

Is this a regression from the previous version?

No but the behavior can cause serious problems for the library users.

Reproducibility

This is an important section. Read it carefully. Failure to do so will cause a 'RTFM' comment.

Just set the current culture to Thai and run some code depends on the code path using such string operations.

Code to reproduce problem:

public void Main()
{
    CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo("th-th");
    // call your library here with operation exercising the code path using such string operation
}
  • I attached a sample spreadsheet. (You can drag files on to this issue)
@jahav
Copy link
Member

jahav commented Sep 19, 2022

Related: culture to the workbook #1367. We should use some sensible culture or the workbook culture for all string operations.

Notes:
Use sensible culture (probably ordinal, but be sure to think it through/case-by-case)

Some might methods might be missing StringComparison parameter be missing (we target netstandard 2.0), enable net6 target during PR development to find missing methods. Don't include the target in the PR. Create extension methods (see Remarks for string.Contains)

@jahav jahav added the bug label Sep 19, 2022
@kenleese
Copy link

In a server scenario it could be useful (when creating or opening a workbook) to specify the appropriate culture as derived (for example) from a user's / client's properties associated with the current request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants