Skip to content

General patches and code clean-up - #21

Merged
lukman-paystack merged 16 commits into
PaystackOSS:masterfrom
TobaniEG:master
Aug 31, 2024
Merged

General patches and code clean-up#21
lukman-paystack merged 16 commits into
PaystackOSS:masterfrom
TobaniEG:master

Conversation

@Tobshub

@Tobshub Tobshub commented Aug 26, 2023

Copy link
Copy Markdown
Contributor

Changes:

  • Removed .DS_Store and added it to .gitignore
  • Added a .prettierrc file for consistent formatting and ran prettier on all the files
  • Removed package.json.preferGlobal option (deprecated as per here)
  • Renamed ./lib/Paystack to ./lib/paystack (fixes CLI failing to start #13)
  • Converted files from commonjs modules to ES modules
  • Converted all uses of global to globalThis (legacy as per here)
  • Fixed function to remove duplicate logic and correctly detect when user is already logged in
  • Fixed logging undefined as an error (fixes Undefined error when logging in #20)

- removed use of `global`(deprecated) with `globalThis`
- called `new vorpal()` instead of `require(vorpal)()`
- in './commands/sample.js' moved `keys` outside of the `init` function and changed it from `let` to `const`
@Tobshub

Tobshub commented Aug 26, 2023

Copy link
Copy Markdown
Contributor Author

I promise it's not a large PR, just the formatting and removing the .DS_Store makes it look large 😂

- fixed detecting already logged in
- fixed issue with undefined errors
- used async/await instead of returning `new Promise`
- removed duplicate loginc in auth.js `init` function and Paystack.js `signIn` function

@lucian-duta lucian-duta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Feedback:

  1. .DS_Store & .gitignore: Excellent initiative in removing system-specific files and ensuring they're not committed in the future.

  2. .prettierrc: A thoughtful addition for ensuring consistent code formatting across the project.

  3. package.json.preferGlobal: Kudos for staying updated with the latest standards and eliminating deprecated options.

  4. Renamed ./lib/Paystack: This is a valuable fix, especially considering it resolves a known issue (#13) with the CLI.

  5. Module Conversions: Transitioning from commonjs to ES modules aligns well with modern JavaScript practices. Great move!

  6. global to globalThis: It's good to see proactive steps for legacy support.

  7. Duplicate Logic: Streamlining code by eliminating repetitive logic always improves maintainability. Well done!

  8. Logging Issue: Addressing the "Undefined error" issue (#20) is crucial for clarity during debugging sessions. Thanks for spotting and fixing it.

Overall Feedback:

These changes look impressive and seem to significantly enhance the project's code quality. Even though I don't have approval rights on this repository, if I did, I'd certainly give it a thumbs up. Great work!

@tolu-paystack tolu-paystack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tolu-paystack

Copy link
Copy Markdown
Contributor

@lukman-paystack can you merge please? I've reviewed the changes and we're good to go

@lukman-paystack lukman-paystack self-assigned this Aug 31, 2024
@lukman-paystack
lukman-paystack self-requested a review August 31, 2024 10:05
@lukman-paystack
lukman-paystack merged commit ea78735 into PaystackOSS:master Aug 31, 2024
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.

Undefined error when logging in CLI failing to start

4 participants