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

[#55] fetch exe_name from api if ommited #76

Closed
wants to merge 2 commits into from
Closed

[#55] fetch exe_name from api if ommited #76

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81e6964
[#55] fetch exe_name from api if ommited
mmohammadi9812 Sep 9, 2022
f6d5160
replace github api request for exe_name with a simple repo clone
mmohammadi9812 Sep 10, 2022
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
39 changes: 37 additions & 2 deletions src/sync/configure.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ pub fn configure_tool(tool_name: &str, config_asset: &ConfigAsset) -> Tool {
fn full_configure(config_asset: &ConfigAsset) -> Option<ToolInfo> {
let owner = config_asset.owner.clone()?;
let repo = config_asset.repo.clone()?;
let exe_name = config_asset.exe_name.clone()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this line is just

    let exe_name = config_asset.exe_name.clone().unwrap_or(config_asset.repo.clone()?);

Can we not let it error and give some type of output to the user to add an exe_name to the config?
This way there is no need for an extra api call.

Another approach would be looking for the common substring in the asset names. But I am more in favor of letting the error bubble up so the tool does not download something if the user forgets to add an exe_name or they mistake the name of the binary with the repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is a small piece of the json returned by the repos/{owner}/repo/release/tags/{tag} endpoint.

The name is always formatted as {name}-{tag}-{arch}.some.zip.extension

  "assets": [
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486868",
      "id": 38486868,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODY4",
      "name": "ripgrep-13.0.0-arm-unknown-linux-gnueabihf.tar.gz",
      "label": "",
     ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 1763861,
      "download_count": 3387,
      "created_at": "2021-06-12T12:31:57Z",
      "updated_at": "2021-06-12T12:31:58Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-arm-unknown-linux-gnueabihf.tar.gz"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486879",
      "id": 38486879,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODc5",
      "name": "ripgrep-13.0.0-i686-pc-windows-msvc.zip",
      "label": "",
    ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 1591463,
      "download_count": 3282,
      "created_at": "2021-06-12T12:32:47Z",
      "updated_at": "2021-06-12T12:32:48Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-i686-pc-windows-msvc.zip"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486907",
      "id": 38486907,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2OTA3",
      "name": "ripgrep-13.0.0-x86_64-apple-darwin.tar.gz",
      "label": "",
   ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 1815615,
      "download_count": 192942,
      "created_at": "2021-06-12T12:35:00Z",
      "updated_at": "2021-06-12T12:35:00Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-x86_64-apple-darwin.tar.gz"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486889",
      "id": 38486889,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODg5",
      "name": "ripgrep-13.0.0-x86_64-pc-windows-gnu.zip",
      "label": "",
     ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 9811405,
      "download_count": 10684,
      "created_at": "2021-06-12T12:33:25Z",
      "updated_at": "2021-06-12T12:33:26Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-x86_64-pc-windows-gnu.zip"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486875",
      "id": 38486875,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODc1",
      "name": "ripgrep-13.0.0-x86_64-pc-windows-msvc.zip",
      "label": "",
     ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 1734357,
      "download_count": 30550,
      "created_at": "2021-06-12T12:32:30Z",
      "updated_at": "2021-06-12T12:32:30Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-x86_64-pc-windows-msvc.zip"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486871",
      "id": 38486871,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODcx",
      "name": "ripgrep-13.0.0-x86_64-unknown-linux-musl.tar.gz",
      "label": "",
    ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 2109801,
      "download_count": 272291,
      "created_at": "2021-06-12T12:32:02Z",
      "updated_at": "2021-06-12T12:32:03Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-x86_64-unknown-linux-musl.tar.gz"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38493219",
      "id": 38493219,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDkzMjE5",
      "name": "ripgrep_13.0.0_amd64.deb",
      "label": null,
     ...
      "content_type": "application/vnd.debian.binary-package",
      "state": "uploaded",
      "size": 1574096,
      "download_count": 71172,
      "created_at": "2021-06-12T17:36:20Z",
      "updated_at": "2021-06-12T17:36:21Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep_13.0.0_amd64.deb"
    }
  ],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we not let it error and give some type of output to the user to add an exe_name to the config?

I don't have many examples in my mind where the tool name is different than repo name and needs explicit user handling, so I don't think that benefits the end user.

The only case where I think manual handling would be beneficial, might be where two or more tools have the same name, or at least same repo name, which I think is a very rare case.

Here is a small piece of the json returned by the repos/{owner}/repo/release/tags/{tag} endpoint.

The name is always formatted as {name}-{tag}-{arch}.some.zip.extension

Most of repos, especially the big ones, have a nice consistent format, but it's not forced and so, not always done consistently

An example of such would be teeldear where assets' names are in {name}-{arch}.* format:

:;curl -sL 'https://api.github.com/repos/dbrgn/tealdeer/releases' | jq '.[] | .assets | .[] | .name' | head -15
"completions_bash"
"completions_fish"
"completions_zsh"
"LICENSE-APACHE.txt"
"LICENSE-MIT.txt"
"tealdeer-linux-arm-musleabi"
"tealdeer-linux-arm-musleabi.sha256"
"tealdeer-linux-arm-musleabihf"
"tealdeer-linux-arm-musleabihf.sha256"
"tealdeer-linux-armv7-musleabihf"
"tealdeer-linux-armv7-musleabihf.sha256"
"tealdeer-linux-i686-musl"
"tealdeer-linux-i686-musl.sha256"
"tealdeer-linux-x86_64-musl"
"tealdeer-linux-x86_64-musl.sha256"

Copy link
Owner

Choose a reason for hiding this comment

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

The original issue #55 was about the ability to not specify the exe_name field when the repo field is specified. repo is a required field (unless the tool is hardcoded in tool-sync, in that case, you don't need to specify anything) so we always know the repo name. This way, we don't even need to query the GitHub API to get the name. We can just use repo from the config.

So I propose to move this discussion and the implementation away from the GitHub API-based implementation toward a simpler design 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I didnt know the release binary names aren't generated.

let exe_name = config_asset
.exe_name
.clone()
.unwrap_or(config_asset.repo.clone()?);
Copy link
Owner

Choose a reason for hiding this comment

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

config_asset.repo.clone()? is already evaluated to repo variable (that has type String) from the previous line so this can be simplified:

Suggested change
.unwrap_or(config_asset.repo.clone()?);
.unwrap_or(repo);

let tag = config_asset
.tag
.clone()
Expand Down Expand Up @@ -66,7 +69,7 @@ impl ToolInfo {
exe_name: config_asset
.exe_name
.clone()
.unwrap_or_else(|| self.exe_name.clone()),
.unwrap_or(config_asset.repo.clone().unwrap()),
Copy link
Owner

@chshersh chshersh Sep 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, apparently this shouldn't change at all. self.exe_name is already String (not optional) so it should always be the result unless explicitly overridden in the config

asset_name: AssetName {
linux: config_asset
.asset_name
Expand Down Expand Up @@ -219,6 +222,38 @@ mod tests {
);
}

#[test]
fn empty_exe_name() {
let tool_name = "abcdef";

let config_asset = ConfigAsset {
owner: Some(String::from("chshersh")),
repo: Some(String::from("tool-sync")),
exe_name: None,
asset_name: AssetName {
linux: Some(String::from("my-linux")),
macos: Some(String::from("my-macos")),
windows: Some(String::from("yours-windows")),
},
tag: Some(String::from("1.0.0")),
};

assert_eq!(
configure_tool(tool_name, &config_asset),
Tool::Known(ToolInfo {
owner: "chshersh".to_string(),
repo: "tool-sync".to_string(),
exe_name: "tool-sync".to_string(),
asset_name: AssetName {
linux: Some("my-linux".to_string()),
macos: Some("my-macos".to_string()),
windows: Some("yours-windows".to_string()),
},
tag: ToolInfoTag::Specific("1.0.0".to_string()),
})
);
}

#[test]
fn partial_override() {
let tool_name = "ripgrep";
Expand Down