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

Remove json field names #120

Merged
merged 1 commit into from
Jun 12, 2020
Merged

Remove json field names #120

merged 1 commit into from
Jun 12, 2020

Conversation

iomodo
Copy link
Contributor

@iomodo iomodo commented Jun 3, 2020

Summary

This PR will remove json field names in autolink struct.

Ticket Link

Fixes #115

@iomodo iomodo requested a review from levb as a code owner June 3, 2020 10:28
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2020

Codecov Report

Merging #120 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #120   +/-   ##
=======================================
  Coverage   48.09%   48.09%           
=======================================
  Files           6        6           
  Lines         576      576           
=======================================
  Hits          277      277           
  Misses        282      282           
  Partials       17       17           
Impacted Files Coverage Δ
server/autolink/autolink.go 51.06% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc1c8da...4880ffa. Read the comment docs.

@iomodo iomodo requested a review from hanzei June 3, 2020 10:28
@iomodo iomodo added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jun 3, 2020
@iomodo iomodo added this to the v1.2.1 milestone Jun 3, 2020
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

One suggestion, but otherwise LGTM

WordMatch bool `json:"wordmatch"`
DisableNonWordPrefix bool `json:"disable_non_word_prefix"`
DisableNonWordSuffix bool `json:"disable_non_word_suffix"`
Name string
Copy link
Contributor

Choose a reason for hiding this comment

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

0/5 maybe keep the json tags to ensure they stay the same in the future

Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

Oops cc @crspeller Thanks for fixing!

@levb levb requested a review from DHaussermann June 3, 2020 22:00
@levb levb removed the 2: Dev Review Requires review by a core committer label Jun 3, 2020
@DHaussermann
Copy link

@iomodo I seem to be getting results that are quite different from the expected 1.1.1 behavior oulined here #115 (comment)

I have deployed the code change from this PR and my config looks like this (note the chevrons <> get changed when saved into the config.)

                "links": [
                    {
                        "DisableNonWordPrefix": true,
                        "DisableNonWordSuffix": false,
                        "Disabled": false,
                        "Name": "link1",
                        "Pattern": "(?P\u003cmmprefix\u003e^|\\\\s|(?:^| )\\\\()(?P\u003curl\u003ehttps://test\\\\.service/(?P\u003cn\u003eTEST\\\\d{7}))",
                        "Scope": null,
                        "Template": "${mmprefix}[${n}](${url})",
                        "WordMatch": false
                    },
                    {
                        "DisableNonWordPrefix": true,
                        "DisableNonWordSuffix": false,
                        "Disabled": false,
                        "Name": "link2",
                        "Pattern": "(?P\u003cmmprefix\u003e^|\\\\s|(?:^| )\\\\()(?P\u003cn\u003eTEST\\\\d{7})",
                        "Scope": null,
                        "Template": "${mmprefix}[${n}](https://test.service/${n})",
                        "WordMatch": false
                    }
                ],

but I get the output here
Screen Shot 2020-06-11 at 12 39 44 AM

It looks like the input does not match the patterns and so auto-link is not doing anything at all. Any thoughts?

@iomodo
Copy link
Contributor Author

iomodo commented Jun 11, 2020

@DHaussermann the problem might be the pattern string, it has too many slashes. Try these:

"Pattern": "(?P\u003cmmprefix\u003e^|\\s|(?:^| )\\()(?P\u003curl\u003ehttps://test\\.service/(?P\u003cn\u003eTEST\\d{7}))",

"Pattern": "(?P\u003cmmprefix\u003e^|\\s|(?:^| )\\()(?P\u003cn\u003eTEST\\d{7})",

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Tested the input provided in the related issue and I see the desired output
  • Tested to ensure enabling word match does not garble results with the sample input as mentioned in the issue
  • Added a test for DisableNonWordPrefix to release testing to cover this case
  • Regression tested the feature here 8c82b7d that introduced the bug. No issues found with Jira plugin support.
    LGTM!

Thanks @iomodo for the help on this one.

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jun 11, 2020
@hanzei hanzei mentioned this pull request Jun 11, 2020
@iomodo iomodo merged commit 1a6ac33 into master Jun 12, 2020
@iomodo iomodo deleted the gh-155-behavior-change branch June 12, 2020 12:39
@hanzei hanzei mentioned this pull request Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autolinking behavior changed between 1.1.1 and 1.2.0
5 participants