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 bump version command that creates a git tag and a bower.json file (issue #203) #289

Merged
merged 38 commits into from Jul 7, 2019

Conversation

@Dretch
Copy link
Collaborator

commented Jun 23, 2019

Description of the change

Implements #203. Not yet feature complete.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

@Dretch Dretch changed the title Issue 203 bump version command Add bump version command that creates a git tag and a bower.json file (issue #203) Jun 23, 2019

@Dretch

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 23, 2019

It looks like there are quite a few conflicts with #272 -- once that is merged I will resolve them.

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

@Dretch this looks quite good already, while I think #272 will need some more work, so I'd say we could try to merge this first and then rebase that one? (cc @jonathanlking)

What's missing from here to be feature complete?

@jonathanlking

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

@Dretch @f-f please go ahead and merge this first - I’m very happy to rebase and I still need to do some work.

@Dretch

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

@f-f

What's missing from here to be feature complete?

Right now I thinking of:

  • Taking the license and repository field from spago.dhall and putting them in the generated bower.json.
  • Readme and --help message tweaks
  • Moving all messages into Messages.hs
  • Maybe handling hash and branch versions from the package set (right now I think this will just create a broken bower.json)
  • Maybe running bower install after generating the bower.json, as a sanity check.
  • Maybe more tests.
@f-f

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@Dretch thanks for the summary. Some suggestions:

Taking the license and repository field from spago.dhall and putting them in the generated bower.json

For doing this let's add an ensurePublishConfig nearby here that tries to conform to a new PublishConfig type

Moving all messages into Messages.hs

As time goes I'm less sure the Messages module is useful, as you lose a bit of context if the relevant error message is one indirection away. So far the general attitude in my PRs has been to only put there multiline messages, and keep short ones in the relevant places. But I'd say to not worry much about all of this

Maybe handling hash and branch versions from the package set (right now I think this will just create a broken bower.json)

Right, great point. Fortunately we have some facilities for this already 🎉
You could use getMetadata (we already use this for the global cache) to get a map containing (among other things) a list of tags for every package in the set, so you could just check if all the refs are in there.

f-f and others added some commits Jun 28, 2019

@Dretch

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 29, 2019

Thanks for the feedback @f-f

For doing this let's add an ensurePublishConfig nearby here that tries to conform to a new PublishConfig type

I will look at doing this.

As time goes I'm less sure the Messages module is useful, as you lose a bit of context if the relevant error message is one indirection away. So far the general attitude in my PRs has been to only put there multiline messages, and keep short ones in the relevant places. But I'd say to not worry much about all of this

Okay, great.

Right, great point. Fortunately we have some facilities for this already. You could use getMetadata (we already use this for the global cache) to get a map containing (among other things) a list of tags for every package in the set, so you could just check if all the refs are in there.

Okay, I'll look at getMetadata. There are two options I can think of for handling non-tagged versions, though:

  1. Fail when generating the bower.json if the package set refers to any such versions.
  2. Generate a bower.json with Git URL versions, like:
dependencies: {
  "mkdirp": "https://github.com/joshuahhh/purescript-mkdirp.git#master"
}

Do you have any preference?

EDIT: Also, it looks like it will be necessary to call bower install, because otherwise pulp publish complains it can't read the resolved dependency versions from bower_components.

Dretch and others added some commits Jul 2, 2019

@Dretch
Copy link
Collaborator Author

left a comment

🤷‍♂

@f-f

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@Dretch yeah, I also have no idea what's going on. The closest thing I found is this issue, that mentions that inproc could be a problem here (we use inproc for Git and procs for Bower)

@f-f f-f referenced this pull request Jul 4, 2019
10 of 10 tasks complete
@f-f

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

@Dretch yeah it looks like 3c14203 fixed it 🎉

Dretch added some commits Jul 6, 2019

Move the bump-version command into a new "Publish commands" group.
"Publish commands" comes before "Psc-Package compatibility commands"
because it will probably be used more often.
Don't commit the generated bower.json during `spago bump-version`.
Instead, ask the user to commit it and then re-run the command.
Fix the package set version, license, repository and extra package, i…
…n the BumpVersionSpec setup function, rather than versioning a whole test project.

@Dretch Dretch force-pushed the issue-203-bump-version-command branch from 9441eeb to d4cd25f Jul 6, 2019

@f-f f-f marked this pull request as ready for review Jul 6, 2019

@f-f

f-f approved these changes Jul 6, 2019

Copy link
Member

left a comment

@Dretch looks great, thanks for doing this! 👏🎉

Ping @hdgarrood for a quick review, but I'd say we should merge already (so we can rebase #301, #272 and #283 on this) and address feedback in subsequent PRs

@hdgarrood

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

This looks reasonable to me, but if I am reading correctly I think this is currently assuming that a package called foo in your package set is always the same as the package purescript-foo on Bower? That's only a safe assumption if you're using an official package set with no additions or overrides, right?

@hdgarrood

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

(Not that this should prevent this from being merged, just something that's probably worth being aware of.)

@f-f

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

@hdgarrood yeah that's right. I think this assumption is reasonable as we use it around already (e.g. when suggesting deps names), though your comment made me realise that with this implementation you'd be allowed to publish a package that only depends on packages in the official set because we search for every dependency's tag in the "packages metadata", which is a big map containing all tags and commits for all packages in the official package set

So maybe we should just generate the bower file and try to install and see if bower complains? (cc @Dretch)

@hdgarrood

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

So maybe we should just generate the bower file and try to install and see if bower complains? (cc @Dretch)

Oh yes of course, this PR does this already -- it's necessary for pulp publish to succeed -- so that should be fine.

@Dretch Dretch merged commit 75a3407 into master Jul 7, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@Dretch Dretch deleted the issue-203-bump-version-command branch Jul 7, 2019

@f-f

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

@hdgarrood yeah, what I meant there is that we should remove the check here otherwise one wouldn't be able to publish anything that doesn't depend from the packages in the package set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.