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 8 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

@pikurasa
Copy link
Collaborator

@Ashrafmuhmed perhaps you can look into why the "SaveInterface.test.js" test is failing?

@walterbender
Copy link
Member

I suspect a rebase would fix it.

@walterbender
Copy link
Member

But I still have my doubts about making these changes. We should be using the min version of the code here and let the upstream un-minimized version be where users go to explore how it works.

@Ashrafmuhmed
Copy link
Contributor Author

Understood. If we use the minified version here, where should we place the unminimized version for users to explore? Any suggestions?

@walterbender
Copy link
Member

Maybe we should add a README.md file to the lib directory that explains what min files are and how to find the upstream versions? But no need to keep the expanded versions here.

@Ashrafmuhmed
Copy link
Contributor Author

Agreed. Let's add the README.md and clarify everything there. Should we also create a folder named upstream-version to include the unminified code for reference? We can start making these changes right away.

Copy link

github-actions bot commented Mar 2, 2025

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

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