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

readme: add description of ECMAScript #62

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Conversation

unikounio
Copy link
Contributor

@unikounio unikounio commented Mar 11, 2024

Added a description of ECMEScript to the readme and adjust the sample files to match.

Issue

Docs: Add sample code ES Modules format to the examples in the README. · Issue #61 · minimistjs/minimist

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
example/parse.js Outdated Show resolved Hide resolved
@unikounio
Copy link
Contributor Author

@ljharb @shadowspawn
Thank you both for your thorough reviews. I have made the necessary updates based on your feedback.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Looks good to me

@shadowspawn
Copy link
Collaborator

Thanks for working through our various comments @unikounio

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.78%. Comparing base (6715df1) to head (98fe3c8).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #62   +/-   ##
=======================================
  Coverage   98.78%   98.78%           
=======================================
  Files           1        1           
  Lines         165      165           
  Branches       70       70           
=======================================
  Hits          163      163           
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Almost there :-)

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@unikounio
Copy link
Contributor Author

@ljharb
Thank you for the thoughtful suggestion. I have addressed it.

@shadowspawn
Copy link
Collaborator

Urk. Failing a couple of lint tests due to using "new" features in the example code (const and modules). To be clear, these would be problems if they were in the actual implementation since we support very old node versions.

My first thought is examples using modern syntax are more appropriate and useful for new users. Is it appropriate to add an override to .eslintrc for the examples folder and allow modern features?

Do you already have a preferred way of approaching this @ljharb ?

@ljharb
Copy link
Member

ljharb commented Mar 15, 2024

yep! add an "overrides" object for example/** in the root eslintrc, and extend @ljharb/eslint-config/node/latest.

@shadowspawn
Copy link
Collaborator

Are you happy to look at this @unikounio , or would you like a hand with the eslint change? (Fairly easy hopefully, but depending on what you already know and what you are interested in learning...)

@unikounio
Copy link
Contributor Author

@shadowspawn
Thank you for your suggestion. I would like to do this ESLint configuration change. Would it be better to include this change in this PR or would it be more appropriate to create a new PR for this change?

@shadowspawn
Copy link
Collaborator

Include in this PR thanks.

@unikounio
Copy link
Contributor Author

Just pushed the changes to apply the latest ESLint config to the examples directory.
Please take a look when you have a moment. Thanks!

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

rebased and squashed to one commit, and fixed formatting (and examples → example in eslintrc)

this LGTM and can be ff-merged once @shadowspawn concurs :-)

@ljharb ljharb added the documentation Improvements or additions to documentation label Mar 18, 2024
Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ljharb ljharb merged commit 1d79fd9 into minimistjs:main Mar 18, 2024
317 checks passed
@unikounio
Copy link
Contributor Author

Thank you both so much for helping me with this PR!
@ljharb and @shadowspawn, I deeply appreciate your support and the valuable feedback you've provided.
I've learned a lot from this experience and am looking forward to any future opportunities to work together again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants