erlef / rebar3_hex

Rebar3 Hex library
Apache License 2.0
101 stars 49 forks source link

Refactor rebar3_hex_publish and support --app on publish #247

Closed starbelly closed 3 years ago

starbelly commented 3 years ago

This commit is mostly a refactor of rebar3_hex_publish. The following rationale is provided for the refactoring:

Refactor rationale

Changes

TODO:

starbelly commented 3 years ago

This is open for review now. The actual set of changes is small if you don't include deletion of stubs. One thing this PR lacks is testing for umbrellas, but we didn't have this already. We do need this, but it might require re-working some of the integration test helpers a good bit, and I didn't want this PR to grow anymore.

More refactoring and splitting out pure functions into other modules should be a part of that, this should happen in a subsequent PR. More tests need to be added as well of course, but that should also be done as another PR IMO.

I'd like to get this one through and then focus on the rest of the breaking changes in other tasks (and the refactoring of those modules) at this point.

paulo-ferraz-oliveira commented 3 years ago

Added ?RAISE_ERROR macro, raises a ?PROVIDER_ERROR

I think you mean ?RAISE.

Refactored publish/* into publish_package/3 and publish_docs/3

I think you mean /4 on both instances.

Added create_tarball/2

I think you mean /1.

(not trying to nitpick, just want to make sure the changelog is proper, if you ever have one)

paulo-ferraz-oliveira commented 3 years ago

Did none of this imply changes to the README? Or will this come at a later date?

starbelly commented 3 years ago

Added ?RAISE_ERROR macro, raises a ?PROVIDER_ERROR

I think you mean ?RAISE.

Correct, will amend.

Refactored publish/* into publish_package/3 and publish_docs/3

I think you mean /4 on both instances.

Correct, will amend.

Added create_tarball/2

I think you mean /1.

Correct, will amend.

(not trying to nitpick, just want to make sure the changelog is proper, if you ever have one)

No worries, and yes, I have been thinking we need to start keeping a changelog. There's at least two ways to do that. Systematically (requires everyone to do commits in the same format) or we just cobble it together and prepend on each release, but yeah fully agree none the less.

starbelly commented 3 years ago

Did none of this imply changes to the README? Or will this come at a later date?

Later. There's still a good number of PRs that need to happen first, but it shall be updated. We'll also have to update the docs. That brings up another issue, which I will make an issue for.

paulo-ferraz-oliveira commented 3 years ago

No worries, and yes, I have been thinking we need to start keeping a changelog. There's at least two ways to do that. Systematically (requires everyone to do commits in the same format) or we just cobble it together and prepend on each release, but yeah fully agree none the less.

I've seen a bunch of ways this is done, in the past, in other projects.

  1. a CHANGELOG.md maintained by hand,
  2. a note on the GitHub releases page, also by hand,
  3. a CHANGELOG.md maintained more or less automatically,
  4. commits that follow a standard (very very easy to get wrong, unless automatically checked upon push - and am sure this would make some uncomfortable to contribute).

I prefer 3. If the PR titles are proper (I wouldn't go as deep as commits, since mostly we're going to squash them to 1 per pull request - right?), it should be fairly easy to generate one. And it doesn't even need a lot of discipline (unless the releaser wants to have as little trouble as possible), since you can always generate it, tweak the titles - if not readable -, then generate again. (to be discussed in another forum, perhaps)

starbelly commented 3 years ago

@tsloughter ping for review