search

melpa / package-build

Tools for assembling a package archive
https://github.com/melpa/melpa
25 stars 34 forks source link

Clearly separate fetching, version detection and checking out #66

Closed tarsius closed 1 year ago

tarsius commented 1 year ago

This is the first in a series of pull-requests leading up to the implementation of a new version number scheme. The changes in these commits are not directly related to the new scheme, but they are required to implement that.

Some of these changes are breaking changes. For that reason I suggest that we do not merge this into master until we also merge the new version number scheme, to avoid making several breaking changes that downstream have to adjust to in quick succession.

However, it would IMO be a good idea to start using these changes on Melpa itself. If there happen to be regressions, then it would be good to discover and fix these before the big switch to the new version number scheme.

Most of the breaking changes in these commits concern internal functions. (But in the past we had learned that some downstream project use internal functions directly, so we still have to be careful.)

milkypostman commented 1 year ago

Some of these changes are breaking changes. For that reason I suggest that we do not merge this into master until we also merge the new version number scheme, to avoid making several breaking changes that downstream have to adjust to in quick succession.

However, it would IMO be a good idea to start using these changes on Melpa itself. If there happen to be regressions, then it would be good to discover and fix these before the big switch to the new version number scheme.

These two statements are confusing together. There are currently two PRs I'm reviewing, are these two PRs (including this one) and #67 OK to merge into mainline MELPA before any version change? I assume they are at least from the review.

tarsius commented 1 year ago

It seems like this PR is not independent of https://github.com/melpa/package-build/pull/67 is that correct?

Yes. The first six commits are the same. This pr then adds one additional commit.

You're saying you want to merge both together or what?

I though it would be a good idea to have a separate pr to discuss the changes in the last commit. It builds on the changes in the previous commits but it does introduce a breaking change, which I felt you might want to discuss in more detail. I did not want that discussion to obscure any discussions about other changes.

I actually think it'd be nice to merge this one ,then look at https://github.com/melpa/package-build/pull/67 again.

That was mostly the idea but I wouldn't let too much time pass between merging the two. Both changes might require downstream changes. Doing it at once would have the benefit that they would not have to look into whether have to make changes twice and could just get it over with.

tarsius commented 1 year ago

These two statements are confusing together. There are currently two PRs I'm reviewing, are these two PRs (including this one) and https://github.com/melpa/package-build/pull/67 OK to merge into mainline MELPA before any version change? I assume they are at least from the review.

The idea was that we (melpa) get to use the new version but that we don't force downstream projects to adapt just yet because the benefits are not all that obvious. But since I have heard about your timeline on rolling out a new version scheme, I have changed my mind about this. Let's land these changes now, and make the new version scheme available at a later time. (But again, let's merge #66 and #67 in quick succession.)

milkypostman commented 1 year ago

also looks good to me.

milkypostman commented 1 year ago

Also, I meant to approve about a week ago and kept getting distracted with life stuff. I suspect with the holidays I should have more time and less all the at the same time.