search

chshersh / tool-sync

🧰 Download pre-built binaries of all your favourite tools with a single command
https://crates.io/crates/tool-sync
Mozilla Public License 2.0
69 stars 16 forks source link

[#116] prevent sha from being included in asset name hits #117

Closed MitchellBerend closed 1 year ago

MitchellBerend commented 1 year ago

Resolves #116 This pr adds filters out the sha substring in the asset name so it does not get trigger a MultipleFound error. It also augments a test to check this behaviour.

Thanks to @slyshykO for finding this bug and reporting it.

Additional tasks

MitchellBerend commented 1 year ago

Maybe its better to target a specific region in the asset name? Something like after the last instance of . so for example shadow.tar.gz still gets recognized as a hit.

slyshykO commented 1 year ago

I propose to add a filter to settings and use a wildcard for filtering

MitchellBerend commented 1 year ago

Do you have suggestions on what this would look like?

slyshykO commented 1 year ago

Do you have suggestions on what this would look like?

[starship] 
owner = "starship"
repo = "starship"
exe_name = "starship"
asset_name.windows = "starship-x86_64-pc-windows-msvc.zip"

asset_name.exclude = ["*.tar.gz", "*.sha256"]
MitchellBerend commented 1 year ago

Can you try and implement this @slyshykO? I don't see a nice way to filter on multiple things without using regex in a loop.

The current filter lives on line 81 of this file: https://github.com/chshersh/tool-sync/blob/dd7a006da995285eb925965472d67f87107d8c19/src/model/tool.rs#L75-L97

slyshykO commented 1 year ago

@MitchellBerend can I add a new dependency https://crates.io/crates/wildmatch?

chshersh commented 1 year ago

Hey, @MitchellBerend @slyshykO, could I ask you not to rush the implementation of this? I would like everyone to agree on the final design before we spent a lot of time implementing code that will be thrown away 🙏🏻

I'll try to provide my thoughts in #50 and I'll mention everyone to request feedback 🙂

As for wildmatch: do you mean to use it to implement a separate asset_name.exclude field? I'm not sure I want to go that way. And wildmatch doesn't support $ so we can solve this problem without a separate field.

I'd rather spend some time thinking about the proper filtering (inclusion/exclusion) mechanism 🤔

slyshykO commented 1 year ago

As for wildmatch: do you mean to use it to implement a separate asset_name.exclude field? I'm not sure I want to go that way.

As an alternative, there is possible to consider an asset's names as wildcards. As an example: *x86_64-pc-windows-msvc.zip .

MitchellBerend commented 1 year ago

I'm closing this pr so others won't see a pr linked to the original issue.