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

Platform Compatibility #28

Merged
merged 13 commits into from
Apr 29, 2024
Merged

Platform Compatibility #28

merged 13 commits into from
Apr 29, 2024

Conversation

LyubomirT
Copy link
Contributor

@LyubomirT LyubomirT commented Apr 28, 2024

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Breaking Change
  • Documentation Update

Description

As discussed in #23, I rewrote the code using Node.js and optimized it for all platforms using platform-independent methods built into Node.js. This has no dependencies and requires nothing but node to be installed (I used v20.11.0 for this, but since there is a deprecation issue, rmSync may not work on older versions. But it's easily resolvable). It does pretty much the same as the original .sh file. HOWEVER, I've noticed that the CITATION.cff file doesn't work properly with the placeholder replacement algorithm, due to how {{PROJECT_NAME}} and {{NAME}} placeholders are formatted in it, so I had to fix that as well. (changed the template). Honestly, I'm not very proud of how I wrote this, so any feedback or criticism is much appreciated. I hope this code works for you!

Related Tickets & Documents

Added/updated tests?

  • Yes
  • No, and this is why: The code itself seems similar to a unit test, so all testing can be done straight from it (but I can implement a separate similar copy as a unit test if needed).
  • I need help with writing tests

[optional] Are there any post deployment tasks we need to perform?

Don't think so.

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    (I trust it should be updated after my code is reviewed)

Copy link
Owner

@caffeine-addictt caffeine-addictt left a comment

Choose a reason for hiding this comment

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

Hey! Amazing work so far.
Just a little maintainability improvements here and there.

setup.js Outdated Show resolved Hide resolved
setup.js Outdated Show resolved Hide resolved
setup.js Outdated Show resolved Hide resolved
setup.js Show resolved Hide resolved
@caffeine-addictt
Copy link
Owner

As for tests, they will be implemented in another PR that closes #26 (currently waiting for this pr)

@caffeine-addictt caffeine-addictt added Type: Enhancement Suggest an improvement for an existing feature. Type: Feature Suggest a new feature. labels Apr 29, 2024
@caffeine-addictt
Copy link
Owner

@LyubomirT I can also send a PR over if you need help with it

@LyubomirT
Copy link
Contributor Author

@LyubomirT I can also send a PR over if you need help with it

No need for that just yet, I'll handle it. Appreciate the feedback nonetheless!

@LyubomirT
Copy link
Contributor Author

Applied all your requested changes, please review the code again, if possible.

Copy link
Owner

@caffeine-addictt caffeine-addictt left a comment

Choose a reason for hiding this comment

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

LGTM!

@caffeine-addictt
Copy link
Owner

I'll go ahead and update linting CI before merging

caffeine-addictt added a commit that referenced this pull request Apr 29, 2024
@LyubomirT
Copy link
Contributor Author

I'll go ahead and update linting CI before merging

Okay! Take your time.

@caffeine-addictt
Copy link
Owner

caffeine-addictt commented Apr 29, 2024

I'll go ahead and update linting CI before merging

Okay! Take your time.

Looks like merging into main won't affect the CI here.
Could you merge main into your branch and run npm run lint:fix? (As it seems like prettier doesn't like setup.js)

@LyubomirT
Copy link
Contributor Author

I'm not sure if I did this correctly, but it seems to have worked. Please check if everything is in order?

@caffeine-addictt
Copy link
Owner

Yep! No issues with linting anymore. Just some permission issues for my labeler GHaction 🥲

@caffeine-addictt
Copy link
Owner

Thank you for your contribution!! (●´□`)♡

@caffeine-addictt caffeine-addictt merged commit 82c7edd into caffeine-addictt:main Apr 29, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Suggest an improvement for an existing feature. Type: Feature Suggest a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Platform compatibility
2 participants