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

Rewrite installation and invocation instructions using Composer #140

Merged
merged 3 commits into from
Sep 3, 2021

Conversation

Xymph
Copy link
Collaborator

@Xymph Xymph commented Sep 2, 2021

This PR was prompted by this observation. Some of the text was copied from the AddWiki docs, I hope that is alright.

@samwilson, is this what you had in mind?

@Xymph Xymph force-pushed the 140-document-composer branch from 697ca28 to beea825 Compare September 2, 2021 17:39
@Xymph Xymph requested a review from waldyrious September 2, 2021 17:40
@Xymph Xymph mentioned this pull request Sep 2, 2021
Copy link
Collaborator

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

Nice! I left a couple suggestions inline, let me know what you think.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@Xymph Xymph force-pushed the 140-document-composer branch from beea825 to 40e67ca Compare September 2, 2021 19:48
Copy link
Collaborator

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

LGTM :) Let's see what @samwilson has to say.

Copy link
Collaborator

@samwilson samwilson left a comment

Choose a reason for hiding this comment

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

Looks good to me, reads well. I only have a tiny comment about the version number, and I was going to say that something should be added to the release process — but it's already mentioned in GOVERNANCE.md. :-)

README.md Outdated
"hamstar/Wikimate": "0.15.0"
{
"require": {
"hamstar/Wikimate": "^0.15.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't matter massively before 1.0, but I'd suggest writing this as the slightly more lenient ^0.15. This is because (once the trailing zero becomes a 1 or whatever), if a project depends on two libraries that both depend on Wikimate, there's less chance of version conflicts. This doesn't really matter before 1.0, and anyway it's probably not that likely for multiple libraries to depend on Wikimate, so feel free to ignore this comment! :-)

Copy link
Collaborator Author

@Xymph Xymph Sep 3, 2021

Choose a reason for hiding this comment

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

It doesn't matter massively before 1.0, but I'd suggest writing this as the slightly more lenient ^0.15. This is because (once the trailing zero becomes a 1 or whatever), if a project depends on two libraries that both depend on Wikimate, there's less chance of version conflicts. This doesn't really matter before 1.0, and anyway it's probably not that likely for multiple libraries to depend on Wikimate, so feel free to ignore this comment! :-)

Thanks, I made the change.
But I am sort of wondering if what you wrote applies more to ~1.0 rather than ^1.0 (1.0.0 will indeed be our next release).

Per the documentation my understanding is that ^1.0.0 will allow ">=1.0.0 <2.0.0" and is thus very lenient, while ~1.0.0 allows only ">1.0.0 <1.1.0" and thus indeed increases the risk of a version conflict (so ~1.0 would be better indeed). But I'm new to this, so open to enlightenment. :)

@Xymph Xymph force-pushed the 140-document-composer branch from 40e67ca to 22d0ea4 Compare September 3, 2021 09:00
Copy link
Collaborator Author

@Xymph Xymph left a comment

Choose a reason for hiding this comment

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

The prerelease example PR #81 in GOVERNANCE.md is a bit old, includes some discussion clutter, and doesn't show how to propose a Release Summary. Hence the change to short-and-clean #126.

@waldyrious
Copy link
Collaborator

waldyrious commented Sep 3, 2021

I'll merge in a bit, but first will change the title of the PR to be in the imperative mood, like the commit messages (and unlike the CHANGELOG.md file).

@waldyrious waldyrious changed the title Rewrote installation and invocation instructions using Composer Rewrite installation and invocation instructions using Composer Sep 3, 2021
@waldyrious waldyrious merged commit a527b19 into hamstar:master Sep 3, 2021
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.

3 participants