pkgjs / support

Package Support Format
MIT License
24 stars 7 forks source link

Progress cli support #7

Closed mhdawson closed 4 years ago

mhdawson commented 4 years ago

This depends on https://github.com/pkgjs/support/pull/1 landing first

mhdawson commented 4 years ago

@wesleytodd , @Eomm wanted to get this in to make sure we don't duplicate efforts. There was some discussion around whether we should land in master incrementally or work on the cli branch instead.

I'd vote for incrementally landing on master as I think it will potentially be more visible and I don't see a downside since the the project is just ramping up.

Eomm commented 4 years ago

I agree to squash&merge to give to other people a baseline and a pattern to starts with other issues: a branch is not so viewed 👍

mhdawson commented 4 years ago

Pushed another commit to refactor to share code across commands and add some cli testing for validate (much more is needed)

wesleytodd commented 4 years ago

Oh I rebased and force pushed on that other branch. I also fixed @ljharb's issue that it had a path escape issue. Should probably rebase this on top of that before merging as well!

mhdawson commented 4 years ago

@wesleytodd how long ago did you do that? I rebased earlier today

mhdawson commented 4 years ago

And it seems to say there are no conflicts.

wesleytodd commented 4 years ago

Yesterday! I had that comment typed in an open tab, looks like your changes did not cause it to update the ui. I just hit "comment" with the text I typed yesterday lol.

wesleytodd commented 4 years ago

When we are ready to merge this, we should be sure to squash this and add all the co-author info. https://github.com/pkgjs/support/pull/1#issuecomment-628237049

mhdawson commented 4 years ago

Pushed commits to add validation of the support element in package.json and to support remote fetch for validation.

mhdawson commented 4 years ago

Rebased on https://github.com/pkgjs/support/pull/1.

Next step is to look at modules for prettying up the ajv validation output.

mhdawson commented 4 years ago

@ljharb , @wesleytodd I believe I've integrated the changes we discussed when we last got together to discuss progress so far. I think this should satisfy the MVP and is worth landing before we do a final review to see if we are ready to use it as the MVP for promoting the support info as a best practice.

wesleytodd commented 4 years ago

One other comment, the commit messages, can we adopt Conventional Commits? If we do, then I can go ahead with another effort I planned, which was to integrate either Release Please or at least an auto generated change log and version script like standard-version.

I think the change here would be to git rebase -i and edit each commit to have fix or feat in it.

ljharb commented 4 years ago

i'm super opposed to conventional commits spec, because i find the use of the word "chore" to be hugely toxic - i'm fine with the concept (tooling to enforce commit message conventions), but afaik the tooling isn't customizable in that way?

wesleytodd commented 4 years ago

Interesting, I haven't thought through that lens. I agree that chore is not great, but that doesn't technically come from the conventional commit spec, that is an angular thing. I am sure we could get changes to that terminology, would release do?

ljharb commented 4 years ago

@wesleytodd it's usually used for updating dependencies, fixing config files - things that aren't tied to a release or test/impl contents, but are still critically important for operation of the repo. My personal convention uses [Deps], [Dev Deps], [Tests], [Meta], etc, rather than lumping them all under one category.

wesleytodd commented 4 years ago

Cool, so I am not super opinionated on what sub categories we use, the spec leaves that open ended. So examples from yours above would be:

fix(tests): test were broken
fix(meta): updated docs
feat(deps): updated foo@1.2.3 adds support for feature X
fix(dev deps): updated bar@1.2.3 better perf for tests
fix(deps)!: updated foo@2.0.0 breaking change, we expose foo's api to end users
mhdawson commented 4 years ago

@wesleytodd I've pushed a commit to address all of your comments except for the commit comments.

I tried the rebase but it complained about problems being able to apply commits, possibly due to changes pushed after my last change (seems like you might have forced pushed?). Since Github says the PR can be landed I suggest we do that and then I can re-checkout and update the commit comments and the force push to update.

If you agree I'll go ahead and to that.

@helio-frota I've also fixed up your comment so I'll dismiss your request changes.

mhdawson commented 4 years ago

actually seems that if I try to merge without squashing if does say there are problems :(

mhdawson commented 4 years ago

Ok took a bit of work cherry-picking manually but have fixed it up.

@helio-frota, @wesleytodd all of your comments should be addressed.

mhdawson commented 4 years ago

@wesleytodd pushed additional commit to address new comments.

mhdawson commented 4 years ago

I was thinking of leaving the commits as they show the build up of the commands/functionality/testing so the history might be interesting. (Hopefully for subsequent commits we can get code into the repo in smaller chunks and squashing would make sense)

mhdawson commented 4 years ago

Going to land. I believe I addressed @helio-frota comment and he mentioned it was not blocking in the discussion.