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

rust/cargo: more comprehensive support #1292

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

rust/cargo: more comprehensive support #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22faa70
Cargo: Support TOML inline table and cargo dev-dependencies
chawyehsu May 8, 2021
20ac2dd
handle target dependencies
chawyehsu May 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 29 additions & 0 deletions e2e/fixtures/rust/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,32 @@ env_logger = "0.5"

# @OctoLinkerResolve(https://github.com/tokio-rs/bytes)
bytes = "0.5"

# @OctoLinkerResolve(https://github.com/tokio-rs/slab)
slab = { version = "0.4.1", optional = true } # Backs `DelayQueue`

[dev-dependencies]
# @OctoLinkerResolve(https://github.com/tokio-rs/tokio)
tokio = { version = "1.0", features = ["full"] }

# @OctoLinkerResolve(https://github.com/tokio-rs/tokio)
tokio-stream = { version = "0.1", path = "../tokio-stream" }

# @OctoLinkerResolve(https://github.com/async-rs/async-std)
async-std = "1.4"

[build-dependencies]
# @OctoLinkerResolve(https://github.com/rust-lang/pkg-config-rs)
pkg-config = "0.3.7"
# @OctoLinkerResolve(https://github.com/alexcrichton/cc-rs)
cc = { version = "1.0.43", features = ['parallel'] }

[target.'cfg(windows)'.dependencies]
# @OctoLinkerResolve(https://github.com/retep998/winapi-rs)
winapi = "0.3.9"

[target.'cfg(unix)'.dependencies]
# @OctoLinkerResolve(https://github.com/sfackler/rust-openssl)
openssl-sys = { version = "0.9", optional = true }
# @OctoLinkerResolve(https://github.com/sfackler/rust-openssl)
openssl = "1.0.1"
23 changes: 22 additions & 1 deletion packages/plugin-rust-cargo/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,29 @@ function linkDependency(blob, key, value) {
function processTOML(blob, plugin) {
let results = [];
const json = TOML.parse(blob.toString());
// target.*.[dependencies|dev-dependencies|build-dependencies]
// https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
let targets = {};
Object.entries(json["target"]).forEach(([_, obj]) => {
targets = Object.assign(
obj["dependencies"],
obj["dev-dependencies"],
obj['build-dependencies']
);
})

const entries = Object.assign(
json['dependencies'],
json['dev-dependencies'],
json['build-dependencies'],
targets
);

Object.entries(entries).forEach(([name, value]) => {
const version = typeof value === "string" ? value : value.version;
Copy link
Member

Choose a reason for hiding this comment

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

The regex being used (tomlRegExKeyValue) doesn't seem to work with object values like this. I had to add another regex that handled object values like this:

export function tomlRegExObjectKeyValue(key, value) {
  const regexKey = escapeRegexString(key);
  const regexValue = escapeRegexString(value);

  return new RegExp(`(${regexKey})\\s*=\\s*{.*=\\s*"(${regexValue})"`, 'g');
}

And then call this based on if value is a string or object.

This is the full change:

@ -1,21 +1,27 @@
 import * as TOML from '@iarna/toml';
 import insertLink from '@octolinker/helper-insert-link';
-import { tomlRegExKeyValue } from '@octolinker/helper-regex-builder';
+import {
+  tomlRegExKeyValue,
+  tomlRegExObjectKeyValue,
+} from '@octolinker/helper-regex-builder';
import liveResolverQuery from '@octolinker/resolver-live-query';

-function linkDependency(blob, key, value) {
-  const regex = tomlRegExKeyValue(key, value);
+function linkDependency(blob, key, value, isStringMatch) {
+  const regex = isStringMatch
+    ? tomlRegExKeyValue(key, value)
+    : tomlRegExObjectKeyValue(key, value);
  return insertLink(blob, regex, this);
}

function processTOML(blob, plugin) {
  let results = [];
  const json = TOML.parse(blob.toString());
  const entries = Object.assign(json['dependencies'], json['dev-dependencies']);

  Object.entries(entries).forEach(([name, value]) => {
-    const version = typeof value === "string" ? value : value.version;
-    const linked = linkDependency.call(plugin, blob, name, version);
+    const isString = typeof value === 'string';
+    const version = isString ? value : value.version;
+    const linked = linkDependency.call(plugin, blob, name, version, isString);

    results = results.concat(linked);
  });
@ @@

@stefanbuck thoughts on this?

Copy link
Author

Choose a reason for hiding this comment

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

How about passing the isStringMatch to tomlRegExKeyValue instead of creating a new regex function.

export function tomlRegExKeyValue(key, value, isString) {
  const regexKey = escapeRegexString(key);
  const regexValue = escapeRegexString(value);
  const reStr = isString ? `(${regexKey})\\s*=\\s*"(${regexValue})"`
    : `(${regexKey})\\s*=\\s*{.*=\\s*"(${regexValue})"`;

  return new RegExp(reStr, 'g');
}
function linkDependency(blob, key, value, isString) {
  const regex = tomlRegExKeyValue(key, value, isString);
  return insertLink(blob, regex, this);
}

function processTOML(blob, plugin) {
  let results = [];
  const json = TOML.parse(blob.toString());
  const entries = Object.assign(json['dependencies'], json['dev-dependencies']);

  Object.entries(entries).forEach(([name, value]) => {
    const isString = typeof value === "string";
    const version = isString ? value : value.version;
    const linked = linkDependency.call(plugin, blob, name, version, isString);

    results = results.concat(linked);
  });

  return results;
}

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late response. I think both are valid approaches and at the end it doesn't really matter. I don't see this toml regex being used in other places than plugin-rust-cargo/index.js. Unless @xt0rted disagrees I think we can go ahead with the single function suggestions from @chawyehsu

Copy link
Author

Choose a reason for hiding this comment

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

I prefer the single function, thoughts? @xt0rted

Copy link
Member

Choose a reason for hiding this comment

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

I prefer two functions since it's clearer what's goin on, but either way is fine with me.

// filter entries don't have version
if (!version) return;

Object.entries(json.dependencies).forEach(([name, version]) => {
const linked = linkDependency.call(plugin, blob, name, version);

results = results.concat(linked);
Expand Down