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

Refactor webL10n.js for Improved Readability and Maintainability #4445

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Ashrafmuhmed
Copy link
Contributor

Description

This pull request refactors the webL10n.js file to improve readability and maintainability. The following changes have been made:

  • Fixed formatting (spacing, indentation).
  • Renamed unclear variables for better understanding.
  • Split large functions into smaller, reusable ones.

Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
turtle-singer.test.js

@walterbender
Copy link
Member

We need to think about whether or not we want to maintain these changes locally. But it does make the code easier to read.

@Ashrafmuhmed
Copy link
Contributor Author

I understand that the i10n library might change in the near future. However, since I’m very interested in this idea, I believe these refactoring changes will make it easier for anyone working on this part of the project to understand the existing implementation.

Also, I noticed that webL10n.sugarizer.js could benefit from similar refactoring, so I’ve also started working on improving it. Let me know if you have any thoughts on this!

@walterbender
Copy link
Member

OK. I'll test these changes.

@Ashrafmuhmed
Copy link
Contributor Author

Ashrafmuhmed commented Feb 24, 2025

just following up, any progress on the review?
@walterbender

Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
turtle-singer.test.js

@Ashrafmuhmed Ashrafmuhmed deleted the trial branch February 24, 2025 21:26
@Ashrafmuhmed Ashrafmuhmed restored the trial branch February 24, 2025 21:27
@Ashrafmuhmed Ashrafmuhmed reopened this Feb 24, 2025
@Ashrafmuhmed
Copy link
Contributor Author

Sorry about that, this commit should be in another branch under working.

Copy link

✅ All Jest tests passed! This PR is ready to merge.

Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

SaveInterface.test.js

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.

2 participants