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

Change markdown pattern in LinkFormatter to fix parens in url #106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabsrc
Copy link

@fabsrc fabsrc commented Feb 16, 2019

Fixes #105

Changed the regexp pattern for markdown links to work correctly with parenthesis.

Also see: https://rubular.com/r/WfdZ1arvF6PNWO

Copy link

@paprikati paprikati left a comment

Choose a reason for hiding this comment

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

LGTM - would like that commit message to clarify why you're removing that step though as it seems irrelevant to the PR.
(have confirmed this is still a bug, btw)

.travis.yml Outdated
@@ -3,10 +3,6 @@ sudo: false
cache: bundler
bundler_args: "--without development"

before_install:

Choose a reason for hiding this comment

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

can you explain why you are doing this in the commit message?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I opened this PR quite a while ago, but I think the issue was, that the build was failing: https://travis-ci.org/github/stevenosloan/slack-notifier/builds/494350577

When I removed these lines, the build was green again: https://travis-ci.org/github/stevenosloan/slack-notifier/builds/494352716

I checked the Travis docs an apparently these lines are required if Bundler 2.x should be used in older Ruby versions (https://docs.travis-ci.com/user/languages/ruby/#bundler-20). Not sure if that is really required though.

Choose a reason for hiding this comment

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

would be good to rebase and verify that the build still fails without this - maybe worth pulling into a separate PR if it does?

Copy link
Author

Choose a reason for hiding this comment

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

Dropped the commit and the build still fails: https://travis-ci.com/github/fabsrc/slack-notifier/builds/181107631

But if it can be merged with the failing build, it's fine for me.

Choose a reason for hiding this comment

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

I'm new to getting involved with this repo - gonna try and chat to the current maintainer and see how we can get this merged (probably with the travisCI fix one way or another). thanks for your patience - I'll get back to you when I can

@stevenosloan stevenosloan changed the base branch from master to main May 7, 2021 17:41
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.

None yet

2 participants