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.

Sign up for GitHub

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

Jump to bottom

Add support for .net global tool configs #807

Merged
merged 2 commits into from
Jan 30, 2020
Merged

Add support for .net global tool configs #807

merged 2 commits into from
Jan 30, 2020

Conversation

xt0rted
Copy link
Member

@xt0rted xt0rted commented Jan 29, 2020

Checklist:

  • If this PR is a new feature, please provide at least one example link
  • Make sure all of the significant new logic is covered by tests

Sample files

The changes to the regex builder are needed to allow turning off the g flag because some tools have commands of the same name, such as in the second link, and both instances of dotnet-format were being linkified.

@xt0rted xt0rted marked this pull request as ready for review January 30, 2020 04:05
@xt0rted xt0rted requested a review from stefanbuck January 30, 2020 04:05
stefanbuck
stefanbuck requested changes Jan 30, 2020
View reviewed changes
Copy link
Member

@stefanbuck stefanbuck left a comment

Choose a reason for hiding this comment

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

Just one small thing, sorry

packages/helper-regex-builder/index.js Outdated
}

export function jsonRegExSingleKeyValue(key, value) {
return regexBuilder(key, value, true, false);
Copy link
Member

Choose a reason for hiding this comment

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

Personally I would prefer adding another argument to jsonRegExKeyValue instead of introducing another function. Basically how it was until 93d8516#diff-b0829a6c903b7a1200ba3d8cfbf4f331

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually the exact setup I had but I switched to this thinking it'd be easier to maintain. Now it makes sense as to why there's unit tests with a 3rd parameter for those functions even though they only have 2.

Copy link
Member

Choose a reason for hiding this comment

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

Up to you, I see value in both approaches ... at the end it doesn't really matter 😉

Regarding the tests, in #774 I had to change quite a lot so it sneaked through ... sorry for the confusion

@xt0rted xt0rted force-pushed the dotnet-global-tools branch from c643e7d to 4211e68 Compare January 30, 2020 22:22
@stefanbuck stefanbuck merged commit 57b9e45 into OctoLinker:master Jan 30, 2020
@xt0rted
Copy link
Member Author

xt0rted commented Jan 30, 2020 •
edited
Loading

While testing this I noticed that it's turning the // item into a link to nuget.org. I wasn't able to find why this was happening though since // clearly isn't a valid package on the site.

OctoLinker/e2e/fixtures/dotnet/dotnet-tools.json

Line 5 in 57b9e45

"//": "@OctoLinkerResolve(https://www.nuget.org/packages/dotnet-format)",

@stefanbuck
Copy link
Member

The RegExp builder returns the follow RegExp

("//")\s*:\s*"(@OctoLinkerResolve\(https://www\.nuget\.org/packages/dotnet\-format\))"

After applying the RegExp the resolver gets called with ["//", "@OctoLinkerResolve(https://www.nuget.org/packages/dotnet-format)"] where the first element will become the target variable. The url of this element is https://www.nuget.org/packages///.

We could add a conditional to prevent this but maybe not worth adding unnecessary complexity since it's not a real issue. What do you think?

@xt0rted
Copy link
Member Author

xt0rted commented Jan 31, 2020

I can live with it if it's just in the test files. No need to over complicate things for an edge case.

@xt0rted xt0rted deleted the dotnet-global-tools branch February 19, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanbuck stefanbuck stefanbuck approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xt0rted @stefanbuck