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

mmohammadi9812 · 2022-09-09T19:19:37Z

Resolves [#55]

chshersh

Thanks for the contribution!

It's a nice idea and the implementation looks rather small 🤔 Though, I think the approach of requesting this information from GitHub as a separate API request increases the cost of the operation significantly.

I believe the main code that requires changes is in the sync/configure.rs module:

tool-sync/src/sync/configure.rs

Lines 60 to 63 in cb339b0

    
           exe_name: config_asset 
        
               .exe_name 
        
               .clone() 
        
               .unwrap_or_else(|| self.exe_name.clone()),

Here it should take the value of repo if exe_name is None.

Previously, I was also thinking about changing the TOML parsing by specifying the exe_name as repo always (as the original issue says). But now I'm thinking that this can lead to potentially confusing behaviour when users override only repo name and suddenly their executable name changes as well.

So changes in sync/configure.rs is the only thing that is required here (in addition to tests and we can avoid querying the GitHub API 🤗 It's important because there's a pretty strict limit on it 😮‍💨

MitchellBerend · 2022-09-10T11:01:16Z

src/sync/configure.rs

 /// Configure 'ToolInfo' completely from 'ConfigAsset'
 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()?;


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.

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" } ],

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"

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 🙂

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

This resolves issues mentioned by chshersh in chshersh#76

chshersh

Nice, that's exactly what I was looking for 👏🏻

I just have a few suggestions to improve the implementation ✍🏻

chshersh · 2022-09-10T15:35:39Z

src/sync/configure.rs

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


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);

chshersh · 2022-09-10T15:41:14Z

src/sync/configure.rs

@@ -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()),


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

chshersh · 2022-09-13T08:33:44Z

Hey @mmohammadi9812, do you need more help with this PR? I believe it's already done, just a few small things to be change 🙁

Let me know if you have any questions!

This resolves issues mentioned by chshersh in #76

chshersh · 2022-09-20T18:27:54Z

@mmohammadi9812 I'm closing this PR in favour of:

[#55] Don't require 'exe_name' in the configuration #104

I've taken your last commit from this PR and added my changes on top of it. But your contribution still counted! Thanks for taking your time to work on this issue 🤗

[chshersh#55] fetch exe_name from api if ommited

81e6964

Resolves [chshersh#55]

mmohammadi9812 requested a review from chshersh as a code owner September 9, 2022 19:19

chshersh assigned mmohammadi9812 Sep 9, 2022

chshersh requested changes Sep 9, 2022

View reviewed changes

MitchellBerend reviewed Sep 10, 2022

View reviewed changes

replace github api request for exe_name with a simple repo clone

f6d5160

This resolves issues mentioned by chshersh in chshersh#76

chshersh requested changes Sep 10, 2022

View reviewed changes

chshersh pushed a commit that referenced this pull request Sep 20, 2022

replace github api request for exe_name with a simple repo clone

d50898d

This resolves issues mentioned by chshersh in #76

chshersh closed this Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mmohammadi9812 commented Sep 9, 2022

chshersh left a comment

MitchellBerend Sep 10, 2022

MitchellBerend Sep 10, 2022

mmohammadi9812 Sep 10, 2022

chshersh Sep 10, 2022

MitchellBerend Sep 10, 2022

chshersh left a comment

chshersh Sep 10, 2022

chshersh Sep 10, 2022 •

edited

Loading

chshersh commented Sep 13, 2022

chshersh commented Sep 20, 2022

	exe_name: config_asset
	.exe_name
	.clone()
	.unwrap_or_else(\|\| self.exe_name.clone()),

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

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

Conversation

mmohammadi9812 commented Sep 9, 2022

chshersh left a comment

Choose a reason for hiding this comment

MitchellBerend Sep 10, 2022

Choose a reason for hiding this comment

MitchellBerend Sep 10, 2022

Choose a reason for hiding this comment

mmohammadi9812 Sep 10, 2022

Choose a reason for hiding this comment

chshersh Sep 10, 2022

Choose a reason for hiding this comment

MitchellBerend Sep 10, 2022

Choose a reason for hiding this comment

chshersh left a comment

Choose a reason for hiding this comment

chshersh Sep 10, 2022

Choose a reason for hiding this comment

chshersh Sep 10, 2022 • edited Loading

Choose a reason for hiding this comment

chshersh commented Sep 13, 2022

chshersh commented Sep 20, 2022

chshersh Sep 10, 2022 •

edited

Loading