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 minimum working example #1

Merged
merged 23 commits into from
Feb 7, 2023
Merged

Add minimum working example #1

merged 23 commits into from
Feb 7, 2023

Conversation

mattxwang
Copy link
Member

@mattxwang mattxwang commented Jan 24, 2023

This PR is a minimum working example (or MVP) for stylelint-create. Functionally, it has a simple workflow:

  1. check if the user has the right version of node
  2. check if a Stylelint config already exists
  3. check if package.json exists
  4. if all checks pass,
    1. install stylelint and stylelint-config-standard with the package manager's installer
    2. write a message for the user with next steps

In addition, I've:

  • set up the typical "Stylelint stack" of ESLint, Prettier, Remark
  • wrote a simple integration test for existing Stylelint config with Vitest
  • run lint + test as actions

I've been manually testing with node create-stylelint.mjs in the project root, though I'm sure that there are better ways to do this.

I do have a few things I'd like some opinions/help on!

Questions

High-level:

  • should I be writing unit tests as well? or do we think integration tests are comprehensive in this case?
  • I kept everything in one file since I felt that the logic wasn't too complex. Should I break things up (particularly, messages)?
  • many of the ESLint norms we have aren't designed for CLI tools. Should I make adjustments locally (with comments), globally (disable rules in the config), or somewhere else?

Specific:

  • should these integration tests be more comprehensive (ex examine the directory structure / last-modified times)? for now, it's mostly looking at error code + messages.
  • in the tests, my approach to get the filepath for the executable and fixtures is very hacky. I struggled to find documentation on Vitest in the correct way to do this (it seems like most people are using it for Vite, or at least for web apps/server code rather than CLIs). Any thoughts?

Items Left

Let me know if we think any of these are unnecessary, or if I am missing something.

  • write more integration tests:
    • happy path
    • package.json doesn't exist
    • failure during installation (I think I'll have to mock execa for this)
  • add TypeScript
  • test on macOS, Windows (I was planning on doing this through CI)
  • better documentation

I'd love to keep working on this and add other features soon!


Ref: stylelint/stylelint#4269.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thanks for creating this MVP. It seems great as a basis!

.github/workflows/testing.yml Outdated Show resolved Hide resolved
.github/workflows/testing.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
create-stylelint.mjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
test/integration.test.js Outdated Show resolved Hide resolved
test/integration.test.js Outdated Show resolved Hide resolved
test/integration.test.js Outdated Show resolved Hide resolved
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Looking great, added some comments...

LICENSE Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/index.mjs Show resolved Hide resolved
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Looking good!

I've made some suggestions.

I don't think we're far off having this ready. It'd be nice to roll it into the v15 launch.

.gitignore Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
@mattxwang
Copy link
Member Author

Thanks everyone for chiming in - will get to addressing reviews soon! Just to be transparent, won't be able to work on this likely until Friday or the weekend (catching up after travelling for interviews)!

package.json Show resolved Hide resolved
vitest.config.ts Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@mattxwang mattxwang force-pushed the mvp branch 2 times, most recently from 7874582 to 0741651 Compare February 3, 2023 23:45
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

I've left one question, but SGTM. Thanks!

package.json Show resolved Hide resolved
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Looking great!

I've added a minor suggestion.

src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Great work! LGTM 👍🏼

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Awesome stuff. I think we can merge this MVP and then pick up any improvements in new tickets and PRs.

@jeddy3 jeddy3 changed the title Add minimum working example, simple test infrastructure Add minimum working example Feb 7, 2023
@jeddy3 jeddy3 merged commit c6d6ada into main Feb 7, 2023
@jeddy3 jeddy3 deleted the mvp branch February 7, 2023 12:34
@jeddy3 jeddy3 mentioned this pull request Feb 9, 2023
2 tasks
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.

5 participants