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

Add hast-util-merge to the list of utilities #24

Closed
wants to merge 1 commit into from

Conversation

viktor-yakubiv
Copy link

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

I propose an utility that merges two hast documents applying some heruistics. I wrote the utility for my own need and find it being raw. I would appreciate your feedback.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Mar 12, 2024
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Welcome @viktor-yakubiv! 👋
Thanks for sharing!

Some thoughts:

  1. In the readme could you include some additional details? Some ideas:
    1. How to install the package (from NPM or GitHub)
    2. A small contained example of using the package in a script
    3. An example of what input document(s) and the output look like
    4. Description of what the options the plugin supports are
  2. It looks like there are tests, great! some ideas on enhancing these:
    1. include a test script in package.json so others know how to run the tests
    2. include any test dependencies as devDependencies so others know to install them
    3. Consider adding a GitHub action to automatically run and report test status https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-nodejs#building-and-testing-your-code
  3. It looks like this uses JSDoc TypeScript? Also wonderful! Some ideas:
    1. Consider adding a tsconfig file so others can build the types
    2. Consider adding TypeScript as a devDependency so others know what version to use
    3. Consider adding a prepack script that runs if/when you publish to npm
  4. It looks like you have a very consistent code style, nice! Some ideas:
    1. It could be good to add a linter/stylechecker to ensure that is maintained. Prettier/ESLint/XO can be great options.

@ChristianMurphy
Copy link
Member

@viktor-yakubiv friendly ping 🔔
Any questions, comments, or concerns on the above?

@viktor-yakubiv
Copy link
Author

viktor-yakubiv commented Mar 21, 2024

Hi @ChristianMurphy, thank you for the feedback and ping as well 🙂

I am familiar with all of those things and intentionally did not do them:

  • documentation was not in my focus so far, but I could adopt common ecosystem format later
  • ESLint and others would pollute the repository, so I did not include it

I would appreciate feedback about functionality and if you can see the use case for this package, also about software design, package exports, clarity of naming (of both functions and package itself).

@ChristianMurphy
Copy link
Member

documentation was not in my focus so far, but I could adopt common ecosystem format later

Understood, the list in syntax-tree is intended as plugins that other can use.
Part of that, is others need to know if/how to use it.
I don't really understand what it does or why I would use it currently.
You don't necessarily need to match ecosystem documentation exactly, but having a starting point is pretty important to being a usable OSS project.

ESLint and others would pollute the repository, so I did not include it

How would having code quality assurances "pollute" the repository?

I would appreciate feedback about functionality and if you can see the use case for this package, also about software design, package exports, clarity of naming (of both functions and package itself).

Happy to, though that level of feedback needs clarity on what the project's: intent, usage, and how the testing works 😅

@viktor-yakubiv
Copy link
Author

viktor-yakubiv commented Mar 21, 2024

How would having code quality assurances "pollute" the repository?

Currently, this is extra work for the sake of work. I doubt someone ever will contribute to this package. However, if someone does and I face code consistency issues, only then I will provide those.

You can grasp the API from exports at the index.js, there are three functions that have types documented. The use case is documented in the readme.

If this is not enough for the feedback, I will provide extended documentation. Please, highlight which of your points are the must to have this merged. Also, if you doubt that this package should be added to the directory, please bring your concerns ahead so we resolve those or at least I don't waste time on extra documentation.


how the testing works

bun test

@remcohaszing
Copy link
Member

I agree with @ChristianMurphy mostly. Maintainers may use whatever tools they want to maintain a project, but I think it’s good to set some criteria to include packages in this list.

  • Some documentation with at least some instructions on how to use the package.
  • A CI job to run tests. (The tests are already there.)
  • A prepack script and CI job to build the types. (The JSDoc types are already there.)
  • The package should be published to npm.

@wooorm
Copy link
Member

wooorm commented Mar 22, 2024

This list is not a list of everything that ever existed. People can already use npm search and github search. This list is curated by us: tools we suggest folks (including beginners) to use.

I want to recommend projects with thorough docs and tests. Things are going to be maintained for a while. That folks will collaborate on.

@wooorm
Copy link
Member

wooorm commented Apr 23, 2024

Closing for now, thanks!

@wooorm wooorm closed this Apr 23, 2024
@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Apr 23, 2024

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

4 participants