tokuhirom / Minilla

Authorizing tool for CPAN modules
https://metacpan.org/release/Minilla
Other
97 stars 65 forks source link

Additional badges parameters for Travis CI #201

Closed amarnus closed 7 years ago

amarnus commented 7 years ago

To enable this feature, while specifying a CI provider in badges, the user can include additional parameters as a query string:

badges: [ "travis?token=xxxxyyyyzzzz&branch=dev" ]

We can add this support for other CI providers supported by Minilla as well.

tokuhirom commented 7 years ago

Could you write tests and docs?

amarnus commented 7 years ago

Done!

syohex commented 7 years ago

Is following format better than this format ?

badges: [ "travis?token=xxxxyyyyzzzz" ]

I thinks this PR format is difficult to add another parameter. (If we need to add parameters other than token in the future, how do we add them ?)

I think it is easy to add other parameters with above format as below. And order is not important in it.

badges: [ "travis?token=xxxxyyyyzzzz&branch=release" ]
amarnus commented 7 years ago

True. That does make sense. I considered it but felt it would be good to start with something simple. You are right, though - the current format is a bit inflexible (potentially confusing too as it isn't really a query string). I'll update this pull request to support the new format.

amarnus commented 7 years ago

/cc @syohex @tokuhirom

I have updated my pull request. However, there is one critical issue for which I need your help. The TOML parser seems to break when any value has a query string (more specifically the = sign). I'm not sure if this is by design or a bug with TOML::Parser.

For example:

name="Minilla"
badges=["travis?token=xxxyyyzzz"]
authority = "cpan:TOKUHIROM"
module_maker = "ModuleBuildTiny"
TOML error in /home/amar/Minilla/minil.toml: Syntax Error: line:2

badges=["travis?token=xxxyyyzzz"]

This is a blocker as it makes the feature unusable.

amarnus commented 7 years ago

Hey guys, Let me know if there's something that I can do to get this merged. Thanks!

syohex commented 7 years ago

Sorry very too late reply. I think it is TOML parser bug. I have sent PR. https://github.com/karupanerura/TOML-Parser/pull/12

syohex commented 7 years ago

You can avoid its bug by inserting spaces around key.

badges = ["travis?token=xxxyyyzzz"]
amarnus commented 7 years ago

Got it!

Sure, we can wait for the issue to be fixed upstream before merging this PR.

karenetheridge commented 7 years ago

This is a side issue mostly, but I was looking at the Minilla documentation and I couldn't figure out how to use the badges. Just putting the .svg link in the distribution's documentation doesn't seem to be enough -- what do I need to do to activate the service?

On Mon, Nov 7, 2016 at 10:57 PM, Amarnath Ravikumar < notifications@github.com> wrote:

Got it!

Sure, we can wait for the issue to be fixed upstream before merging this PR.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tokuhirom/Minilla/pull/201#issuecomment-259060946, or mute the thread https://github.com/notifications/unsubscribe-auth/AASfy4RbNJeL4D9md-BkUkuurj8piosWks5q8B1rgaJpZM4KXtxW .

syohex commented 7 years ago

Thanks.

Please upgrade TOML::Parser for avoiding that issue.

zakame commented 7 years ago

@karenetheridge you have to add badges in the minil.toml file, like e.g. https://github.com/zakame/hashids.pm/blob/master/minil.toml . Then when building the dist, Minilla will add the badges in the resulting README.md.