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 scriptcs, Cake, and FAKE #1035

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+145 −11
Draft

Add support for scriptcs, Cake, and FAKE #1035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea29130
Add support for scriptcs, Cake, and FAKE
xt0rted Sep 24, 2020
2be7a56
Test out creating failed test annotations
xt0rted Sep 24, 2020
fc58fe0
Checkout pr head commit instead of merge commit
xt0rted Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- uses: actions/setup-node@v1
with:
node-version: '12.x'
Expand Down
37 changes: 26 additions & 11 deletions e2e/automated.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ async function executeTest(url, targetUrl, selector) {
await page.waitForSelector(`${selector}[href$="${targetUrl}"]`);
}

function createAnnotation({ file, lineNumber, ex }) {
// eslint-disable-next-line no-console
console.log(`::error file=${file},line=${lineNumber}::${ex.message}`);
}

describe('End to End tests', () => {
beforeAll(async () => {
if (!process.env.E2E_USER_NAME || !process.env.E2E_USER_PASSWORD) {
Expand Down Expand Up @@ -48,25 +53,35 @@ describe('End to End tests', () => {
});

describe('single blob', () => {
fixtures.forEach(({ url, content, lineNumber, targetUrl }) => {
fixtures.forEach(({ url, file, content, lineNumber, targetUrl }) => {
if (targetUrl === false) {
it(`does not resolve ${content}`, async () => {
await executeTest(
url,
targetUrl,
`#LC${lineNumber} .octolinker-link`,
);
try {
await executeTest(
url,
targetUrl,
`#LC${lineNumber} .octolinker-link`,
);
} catch (ex) {
createAnnotation({ file, lineNumber, ex });
throw ex;
}
});
return;
}

it(`resolves ${content} to ${targetUrl}`, async () => {
if (lineNumber) {
await executeTest(
url,
targetUrl,
`#LC${lineNumber} .octolinker-link`,
);
try {
await executeTest(
url,
targetUrl,
`#LC${lineNumber} .octolinker-link`,
);
} catch (ex) {
createAnnotation({ file, lineNumber, ex });
throw ex;
}
} else {
await executeTest(
url,
Expand Down
20 changes: 20 additions & 0 deletions e2e/fixtures/dotnet/scripting/build.cake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// @OctoLinkerResolve(https://www.nuget.org/packages/Cake.DotNetTool.Module)
#module nuget:?package=Cake.DotNetTool.Module&version=0.4.0

// @OctoLinkerResolve(https://www.nuget.org/packages/Cake.Coveralls)
#addin "nuget:https://api.nuget.org/v3/index.json?package=Cake.Coveralls&version=0.10.1"
Comment on lines +2 to +5
Copy link
Member Author

Choose a reason for hiding this comment

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

@stefanbuck I'm having trouble getting these lines to work as I wanted.

I was trying to make everything after #module & #addin the link so you didn't have to search for the package name in the url, but GitHub's parsing the lines with a full url and everything after // is in a span so no link gets injected.

Is there a way to support making that whole value a link or are we stuck with making just the package name the link?

This is the file I was testing against https://github.com/cake-build/cake/blob/develop/build.cake.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds similar to an issue I'm currently facing getting PHP / laravel support in.

<td>
  <span class="pl-k">use</span><span data-rgh-whitespace="space"> </span><span class="pl-v">Illuminate</span>\<span class="pl-v">Contracts</span>\<span class="pl-v">Container</span>\<span class="pl-v">Container</span>;
</td>

Yesterday, I spent two hours try to come up with a solution without luck. It's mainly a lack of support for such markup in https://github.com/padolsey/findAndReplaceDOMText which we use in the insert-link package.

I've noticed that refined-github is able to wrap the url in your case. If we find a solution we should make sure it doesn't conflict with refined-github which is a widely used extension as well.

Is this a blocker for you? Maybe best to ignore it for now and investigate into it separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

No not a blocker. I can have just the package name be the link until we find a better solution.


// @OctoLinkerResolve(https://www.nuget.org/packages/coveralls.io)
#tool "nuget:https://api.nuget.org/v3/index.json?package=coveralls.io&version=1.4.2"

// @OctoLinkerResolve(https://www.nuget.org/packages/GitVersion.Tool)
#tool "dotnet:https://api.nuget.org/v3/index.json?package=GitVersion.Tool&version=5.1.2"

// @OctoLinkerResolve(<root>/dotnet/scripting/import.csx)
#load import.csx

// @OctoLinkerResolve(<root>/dotnet/scripting/import.csx)
#load "./import.csx"

// @OctoLinkerResolve(<root>/dotnet/scripting/import.cake)
#load "./import.cake"
17 changes: 17 additions & 0 deletions e2e/fixtures/dotnet/scripting/build.fsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#r "paket:
source release/dotnetcore
source https://api.nuget.org/v3/index.json
// @OctoLinkerResolve(https://www.nuget.org/packages/FSharp.Core)
nuget FSharp.Core ~> 4.1
// @OctoLinkerResolve(https://www.nuget.org/packages/System.AppContext)
nuget System.AppContext prerelease
// @OctoLinkerResolve(https://www.nuget.org/packages/Newtonsoft.Json)
nuget Newtonsoft.Json
// @OctoLinkerResolve(https://www.nuget.org/packages/Octokit)
nuget Octokit //"

// @OctoLinkerResolve(<root>/dotnet/scripting/import.csx)
#load import.csx

// @OctoLinkerResolve(<root>/dotnet/scripting/import.csx)
#load "./import.csx"
Empty file added e2e/fixtures/dotnet/scripting/import.cake
View file
Open in desktop
Empty file.
Empty file added e2e/fixtures/dotnet/scripting/import.csx
View file
Open in desktop
Empty file.
5 changes: 5 additions & 0 deletions e2e/fixtures/dotnet/scripting/start.csx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// @OctoLinkerResolve(<root>/dotnet/scripting/import.csx)
#load import.csx

// @OctoLinkerResolve(<root>/dotnet/scripting/import.csx)
#load "./import.csx"
1 change: 1 addition & 0 deletions e2e/generate-fixtures-json.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ function findTests(contents) {

memo.push({
url: `${fixturesRoot}${filePath}`,
file: `e2e${filePath}`,
content: lines[index + 1].trim(),
targetUrl,
lineNumber,
Expand Down
1 change: 1 addition & 0 deletions packages/core/load-plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export { default as DotNetCore } from '@octolinker/plugin-dot-net-core';
export { default as DotNet } from '@octolinker/plugin-dot-net';
export { default as DotNetGlobalTools } from '@octolinker/plugin-dot-net-global-tools';
export { default as DotNetProject } from '@octolinker/plugin-dot-net-project';
export { default as DotNetScripting } from '@octolinker/plugin-dotnet-scripting';
export { default as Rubygems } from '@octolinker/plugin-gemfile-manifest';
export { default as Go } from '@octolinker/plugin-go';
export { default as Haskell } from '@octolinker/plugin-haskell';
Expand Down
1 change: 1 addition & 0 deletions packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"@octolinker/plugin-dot-net-core": "1.0.0",
"@octolinker/plugin-dot-net-global-tools": "1.0.0",
"@octolinker/plugin-dot-net-project": "1.0.0",
"@octolinker/plugin-dotnet-scripting": "1.0.0",
"@octolinker/plugin-gemfile-manifest": "1.0.0",
"@octolinker/plugin-github-actions": "1.0.0",
"@octolinker/plugin-github-codeowners": "1.0.0",
Expand Down
28 changes: 28 additions & 0 deletions packages/helper-grammar-regex-collection/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,3 +268,31 @@ export const NET_PROJ_FILE_REFERENCE = regex`
(Include|Update)=${captureSpacedQuotedWord}
.*/?>
`;

export const NET_SCRIPT_FILE = regex`
\#load
\s+
['"]? # beginning quote
(?<$1>[^'"\s]+) # capture the word inside the quotes
['"]? # end quote
`;

export const NET_SCRIPT_NUGET = regex`
\#(addin|module|tool)
\s+
"?
(dotnet|nuget):
.*
package=
(?<$1>[^'"&\s]+)
.*
"?
`;

export const FAKE_NUGET = regex`
nuget
\s+
['"]? # beginning quote
(?<$1>[^'"\s]+) # capture the word inside the quotes
['"]? # end quote
`;
31 changes: 31 additions & 0 deletions packages/plugin-dotnet-scripting/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import {
FAKE_NUGET,
NET_SCRIPT_FILE,
NET_SCRIPT_NUGET,
} from '@octolinker/helper-grammar-regex-collection';
import nugetResolver from '@octolinker/resolver-nuget';
import relativeFile from '@octolinker/resolver-relative-file';

export default {
name: 'DotNetScripting',

resolve(path, [target]) {
// this reference doesn't resolve to anything
if (target === './.fake/build.fsx/intellisense.fsx') {
return [];
Copy link
Member

Choose a reason for hiding this comment

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

Just to share some insights, you can use undefined as a return value as well. Just saying, no need to change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stefanbuck I just tried return; and return undefined; and both give me an error. I guess that's why I was using an empty array here.

Uncaught (in promise) TypeError: Cannot read property 'registry' of undefined
    at app.js:41806
    at Array.map (<anonymous>)
    at app.js:41795
    at app.js:40329
    at Array.map (<anonymous>)
    at run (app.js:40322)
    at app.js:40355
    at gitHubInjection (app.js:15027)
    at init (app.js:40350)

The error points back to this line

OctoLinker/packages/helper-normalise-resolver-results/index.js

Line 67 in dbaaaa5

if (url.registry) {

Copy link
Member

Choose a reason for hiding this comment

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

Ohh sorry for pointing you into the wrong direction. I guess there is some value in having a consistent return type 😉

}

return [relativeFile({ path, target }), nugetResolver({ target })];
Copy link
Member

Choose a reason for hiding this comment

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

The signature of the resolve handler is resolve(path, target, meta, regExp).

Side note: Meta defaults to an empty object and is used in the context of plugin-npm-manifest.

What I want to talk about is the fourth argument regExp which is the matching RegExp from line 29. In my PHP PR I make use of this to use different resolvers

OctoLinker/packages/plugin-php/index.js

Lines 7 to 13 in 87a137a

resolve(path, [target], meta, regExp) {
if (target.includes('\\')) {
return;
}
if (regExp === PHP_FUNC) {
return liveResolverQuery({
maybe we can do something similar here. This allows use to be more specific and does reduce unnecessary look up in the OctoLinker API.

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 awesome, I'll have to revisit some of the css & dotnet things I was working on because I'm pretty sure I'll be able to simplify them by checking the regex.

},

getPattern() {
return {
pathRegexes: [/\.(cs|fs)x$/, /\.cake$/],
githubClasses: [],
};
},

getLinkRegexes() {
return [FAKE_NUGET, NET_SCRIPT_FILE, NET_SCRIPT_NUGET];
},
};
13 changes: 13 additions & 0 deletions packages/plugin-dotnet-scripting/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "@octolinker/plugin-dotnet-scripting",
"version": "1.0.0",
"description": "",
"repository": "https://github.com/octolinker/octolinker/tree/master/packages/plugin-dotnet-scripting",
"license": "MIT",
"main": "./index.js",
"dependencies": {
"@octolinker/helper-grammar-regex-collection": "1.0.0",
"@octolinker/resolver-nuget": "1.0.0",
"@octolinker/resolver-relative-file": "1.0.0"
}
}