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

Remove problematic euro symbol regex in fi #17578

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

WxnChen11
Copy link
Contributor

Remove complexSymbol euro introduced in fi locale in e5dfd96. It is causing issues with reading of euro currency cells in microsoft Excel.

Link to issue number:

#17471

Summary of the issue:

Euro complexSymbol in fi locale causing euro currency numbers to not be read in microsoft excel.

Description of user facing changes

Affects the reading of euro currency numbers in microsoft excel. The behavior should be in line with that of some common locales such as english and spanish where this regex is not present.

Description of development approach

Followed the advice of seanbudd and removed the regex, after spot-checking to verify that this is likely a finnish-only regex and behavior.

Testing strategy:

Unit tests: It might make sense to add a unit test for the processing of euro symbols in not just finnish but all languages.. I will need a pointer to where these should go if there is a good place.
Manual testing: will require deferral to a an individual who has a working microsoft excel license or subscription.

Known issues with pull request:

Requires testing (manual, maybe unit tests)
Requires UX vetting for fi locale, to verify that euro symbols are not somehow different (?) than other languages

Code Review Checklist:

  • Documentation: (Minor change, I do not think it requires a doc entry)
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons. (breaking release)
  • [ ] Security precautions taken. (symbol mapping change, I would expect no significant runtime changes, but might be a good idea for someone more experienced to check this off.)

@coderabbitai summary

Remove complexSymbol euro introduced in fi locale in e5dfd96. It is causing issues with reading of euro currency cells in microsoft Excel.
@WxnChen11 WxnChen11 requested a review from a team as a code owner January 3, 2025 06:52
@WxnChen11 WxnChen11 requested a review from SaschaCowley January 3, 2025 06:52
@josephsl
Copy link
Collaborator

josephsl commented Jan 3, 2025

Hi,

As this is a locale data issue, may I suggest resolving it via translations workflow instead of a direct pull request?

Thanks.

@WxnChen11
Copy link
Contributor Author

Hi Joseph, thank you for your comment. I'm new to the project. Could you elaborate on what the translations workflow entails?

@WxnChen11
Copy link
Contributor Author

I found https://github.com/nvaccess/nvda/wiki/TranslatingUsingAutomaticProcess, and I requested an invitation to join the group.

@josephsl
Copy link
Collaborator

josephsl commented Jan 3, 2025 via email

@seanbudd seanbudd added this to the 2025.1 milestone Jan 6, 2025
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 6, 2025
@seanbudd
Copy link
Member

seanbudd commented Jan 6, 2025

@josephsl - we are no longer using SVN. This is the correct process for updating symbols files now

@josephsl
Copy link
Collaborator

josephsl commented Jan 6, 2025 via email

@seanbudd
Copy link
Member

seanbudd commented Jan 7, 2025

Could you please also send an email to the translators mailing list notifying the Finnish translators about this change.
Thanks

@seanbudd seanbudd merged commit 8c771f0 into nvaccess:master Jan 7, 2025
3 checks passed
@WxnChen11
Copy link
Contributor Author

Sent an email for approval by moderators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants