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.

Sign up for GitHub

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

Jump to bottom

Refactor and optimize codebase #2

Closed
wants to merge 7 commits into from
Closed

Refactor and optimize codebase #2

wants to merge 7 commits into from

Conversation

RoachxD
Copy link
Contributor

@RoachxD RoachxD commented Aug 6, 2023

This pull request contains several changes to the codebase that aim to improve its readability and performance. The main changes are:

  • Fixed MD001/heading-increment/header-increment rule violation in README.md.
  • Fixed two vulnerabilities in package-lock.json.
  • Combined multiple imports from the same module into a single import statement in index.ts.
  • Removed unnecessary intermediate variables and replaced function declarations with arrow functions in index.ts, context.ts, DateTime.tsx, I18nProvider.tsx, Number.tsx, and Text.tsx.
  • Moved some variables outside of the return statement to improve readability in DateTime.tsx and Number.tsx.
  • Reversed the ternary operator in DateTime.tsx and Number.tsx.
  • Changed the quotes used for the import statements from single to double.
  • Added curly braces to the if statement in context.ts.

Suggest using Object.values instead of isEmpty for micro-optimizing DateTime.tsx and Number.tsx.

These changes do not affect the functionality or the output of the code, but they make it more consistent, concise, and maintainable. I hope you find them useful and welcome any feedback or suggestions. Thank you for your time and consideration.

@RoachxD
fix(README.md): MD001/header-increment
141f8ec 
- Heading levels should only increment by one level at a time.
@RoachxD
fix(package-lock.json): 2 vulnerabilities
18200d2 
- Fixed 1 high and 1 moderate vulnerabilities.
@SanichKotikov
Copy link
Owner

Hi. Thanks, but there are important rules for open source:

  • Make a new issue (ask) before making any changes
  • Don't change the exist formatting (tabs, quotes, etc.)

If you can get the formatting back, I can take a look and possibly accept this PR. Thanks any way.

@RoachxD
Copy link
Contributor Author

RoachxD commented Aug 9, 2023

Hello @SanichKotikov, will do! But first, I'd want to know if the aforementioned formatting include reverting back from arrow functions to function declarations?

@SanichKotikov
Copy link
Owner

@RoachxD It's preferable to use named functions, especially for components, because arrow functions can be marked as unknown/unnamed in some debug tools.

@RoachxD
feat: Add Prettier and configure it
bdeecab 
- Prettier has been added and configured for this project.
- The `.gitignore` file has been updated.
- A new `.prettierignore` file has been created.
@RoachxD
feat: add index.ts to components and update main index.ts
d7c2f0b 
- Created `index.ts` file for `components`.
- Updated main `index.ts` in order to reflect the above addition.
- Sorted imports inside main `index.ts`.
@RoachxD
feat(utils): Optimize isEmpty function
7e6316e 
- Created `utils.ts` and added a micro-optimized version of `isEmpty` from 'i18n-mini/lib/utils', which is ~70% faster.
- Updated `DateTime.tsx` and `Numeric.tsx` components to use the new optimized `isEmpty` function.
@RoachxD
refactor(DateTime, Numeric): Improve code readability
27315d8 
- Defined a new constant for preset values.
- Reversed ternary operator.
- Used nullish coalescing operator instead of the logical OR operator.
@RoachxD
style: Format all files through Prettier
acba27b 
- Formatted all files through Prettier (`npx prettier . --write`).
@RoachxD
Copy link
Contributor Author

RoachxD commented Aug 12, 2023

Do you think I should PR on i18n-mini in regards to the optimized version of isEmpty instead of adding overhead to this?

@SanichKotikov
Copy link
Owner

@RoachxD thanks, but I can't merge this PR. I've asked you to reduce count of changes, but instead of this you add more and more... It's better to keep one thing in one PR.

@SanichKotikov
Copy link
Owner

@RoachxD I've made some updates. Thanks for highlighting some of the issues.

About Object.values and isEmpty, I think, it's better to update it in i18n-mini and keep functional style, for example use some insted of every.

@RoachxD
Copy link
Contributor Author

RoachxD commented Aug 14, 2023

@SanichKotikov I understand, sorry for being that unstructured because of my eagerness!

I will be creating multiple issues and wait for your answers in order to see if it's viable or not to create a PR based on your opinion. Thanks!

@RoachxD RoachxD closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RoachxD @SanichKotikov