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

Add Solidity plugin #1405

Merged
merged 4 commits into from
Nov 7, 2021
Merged

Add Solidity plugin #1405

merged 4 commits into from
Nov 7, 2021

Conversation

lucaperret
Copy link
Contributor

@lucaperret lucaperret commented Nov 4, 2021

Plugin Solidity

Adds support for Solidity's import statement.

See this solidity file as an example.
You should be able to click "./IERC721.sol" within import "./IERC721.sol"; and see that file.

Demo

octolinker-solidity

Todo

  • Update packages/blob-reader fixture to add Solidity support on issues highlighted code?
  • Publish @octolinker/plugin-solidity npm package
  • Add export statement to the packages/core/load-plugins.js
  • Add @octolinker/plugin-solidity to packages/core/package.json

@lucaperret
Copy link
Contributor Author

Hey @stefanbuck, not sure how to configure the TRAVIS_PULL_REQUEST_SLUG and TRAVIS_PULL_REQUEST_BRANCH env var to make the build action working. Could you please help here?

@stefanbuck
Copy link
Member

This looks outdated, we don't use Travis anymore. It might due to the fact you run the tests from a fork and this might doesn't work. I'll have a closer look at your PR tonight.

@xt0rted
Copy link
Member

xt0rted commented Nov 5, 2021

@xt0rted
Copy link
Member

xt0rted commented Nov 5, 2021

@lucaperret there's 2 things missing which is why the tests are failing. The plugin needs to be added to core/package.json and core/load-plugins.js. Once you do that you can use yarn start & yarn chrome-open to browse your test files and they should be linked (just verified it myself).

If you're using Node 17 the yarn scripts won't work without first adding set NODE_OPTIONS=--openssl-legacy-provider && to the beginning of them (this is if you're on Windows, it's similar for macOS & Linux).

@lucaperret
Copy link
Contributor Author

lucaperret commented Nov 5, 2021

@lucaperret there's 2 things missing which is why the tests are failing. The plugin needs to be added to core/package.json and core/load-plugins.js. Once you do that you can use yarn start & yarn chrome-open to browse your test files and they should be linked (just verified it myself).

If you're using Node 17 the yarn scripts won't work without first adding set NODE_OPTIONS=--openssl-legacy-provider && to the beginning of them (this is if you're on Windows, it's similar for macOS & Linux).

Locally I run the tests by adding the following code to the load-plugins.js file:

export { default as Solidity } from '../plugin-solidity';

As the package isn't published yet, I guessed I shouldn't add it the import as @octolinker/plugin-solidity same for the package.json.

@xt0rted
Copy link
Member

xt0rted commented Nov 5, 2021

These don't get published to npm, webpack will resolve them from the local folder when it bundles everything. These two changes are all you should need to make:

index 3199f3a..f5d9d3b 100644
--- a/packages/core/load-plugins.js
+++ b/packages/core/load-plugins.js
@@ -25,6 +25,7 @@ export { default as Ruby } from '@octolinker/plugin-ruby';
 export { default as Rust } from '@octolinker/plugin-rust';
 export { default as RustCargo } from '@octolinker/plugin-rust-cargo';
 export { default as Sass } from '@octolinker/plugin-sass';
+export { default as Solidity } from '@octolinker/plugin-solidity';
 export { default as TypeScript } from '@octolinker/plugin-typescript';
 export { default as Vim } from '@octolinker/plugin-vim';
 export { default as GitHubActions } from '@octolinker/plugin-github-actions';
diff --git a/packages/core/package.json b/packages/core/package.json
index 6c72b41..bbed834 100644
--- a/packages/core/package.json
+++ b/packages/core/package.json
@@ -43,6 +43,7 @@
     "@octolinker/plugin-rust": "1.0.0",
     "@octolinker/plugin-rust-cargo": "1.0.0",
     "@octolinker/plugin-sass": "1.0.0",
+    "@octolinker/plugin-solidity": "1.0.0",
     "@octolinker/plugin-typescript": "1.0.0",
     "@octolinker/plugin-vim": "1.0.0",
     "@octolinker/user-interface": "1.0.0",

getPattern() {
return {
pathRegexes: [/\.sol$/],
githubClasses: ['type-solidity', 'highlight-source-solidity'],
Copy link
Member

Choose a reason for hiding this comment

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

@stefanbuck is the build failing because this is set but no example exists for it in /packages/blob-reader/fixtures/github.com/issue/code.html?

Copy link
Member

Choose a reason for hiding this comment

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

As far as I remember yes. This HTML is just the output from OctoLinker/testrepo#2. I added a solidity snippet at the end which should make this test pass. The fixture are updated on each CI run https://github.com/OctoLinker/OctoLinker/blob/main/packages/blob-reader/scripts/update-fixtures.sh. Just for debugging purpose we store the output within the repo. I'll take care of updating the fixture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefanbuck you missed the ".sol" extension I think

Copy link
Member

@xt0rted xt0rted Nov 5, 2021

Choose a reason for hiding this comment

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

@stefanbuck without the .sol extension an exception is thrown. Will the code snippet support even do anything with this plugin? I'm trying to test it but I'm not having any luck getting it to link anything for me.

Turns out my markdown was wrong, it throws an exception even with the .sol extension.

Copy link
Member

Choose a reason for hiding this comment

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

Using ```sol ... results in highlight-source-gerber. ```solidity ... results in highlight-source-solidity so this should work.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not to fuzzy about adding those classes. The only purpose is to make linking in code snippets work. @lucaperret feel free to leave the githubClasses array empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather prefer to have it working on code snippet.

Copy link
Member

Choose a reason for hiding this comment

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

I need to look into this a bit closer. Unfortunately I don't have capability for this today, but I'll have a look tomorrow evening.

@lucaperret lucaperret marked this pull request as draft November 5, 2021 20:04
@lucaperret lucaperret marked this pull request as ready for review November 5, 2021 20:40
@xt0rted
Copy link
Member

xt0rted commented Nov 5, 2021

When testing a code snippet if the import starts with ./ I get the following exception:

app.js:37107 Uncaught (in promise) TypeError: Path must be a string. Received undefined
    at assertPath (app.js:37107)
    at dirname (app.js:37372)
    at __WEBPACK_DEFAULT_EXPORT__ (app.js:10584)
    at Object.resolve (app.js:10082)
    at app.js:7658
    at Array.forEach (<anonymous>)
    at __WEBPACK_DEFAULT_EXPORT__ (app.js:7650)
    at run (app.js:52853)
    at app.js:52893
    at gitHubInjection (app.js:22052)

I'm not really sure why that's happening though. If you remove the . it goes away so I guess it's something in the file resolver.

If the import is import "foo" then it resolves to https://github.com/jasonLaster/foo

@lucaperret
Copy link
Contributor Author

Correct, the solidity import should start with "./" for relative path, and if not, it is a npm package.
@stefanbuck any idea to resolve the build action?

@stefanbuck
Copy link
Member

When testing a code snippet if the import starts with ./ I get the following exception:

Looks like a regression introduced in an earlier version which seems to affect code snippets only. Code snippets are not covered by end to end tests. I tried updating all fixtures files but then the unit tests start failing. To make this PR pass I pushed just the fixture change for the code snippets.

@stefanbuck stefanbuck merged commit 486b00d into OctoLinker:main Nov 7, 2021
@stefanbuck
Copy link
Member

Thanks for your contribution @lucaperret As mentioned I’m going to look into the snippet issue and update the store version later this week.

@lucaperret
Copy link
Contributor Author

Hey @stefanbuck, when are you planning to release OctoLinker with this feature?

@stefanbuck
Copy link
Member

Thanks for the reminder. I wanted to fix the highlight issue we discovered first, but I'm maxed out with other things right now. Therefore maybe best to just ship it. I'm going to tackle this at some point this week (sorry my schedule is very tight).

@lucaperret
Copy link
Contributor Author

If I can do anything to help you @stefanbuck feel free :)

@stefanbuck
Copy link
Member

Hi @lucaperret, sorry for the delay I was off sick. I just created a new release which will be available very soon. Watch out for v6.10.0.

Thanks again for your contribution and taking the time adding Solidity support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants