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 Shorthand for Install Command #43

Closed
wants to merge 3 commits into from
Closed

Add Shorthand for Install Command #43

wants to merge 3 commits into from

Conversation

hadihammurabi
Copy link

@hadihammurabi hadihammurabi commented Oct 5, 2021

use username/repository for github
use ./example.AppImage for local file with relative path
use /dir/example.AppImage for local file with absolute path

example:
image

use username/repository for github
use ./example.AppImage for local file with relative path
use /dir/example.AppImage for local file with absolute path
@vercel
Copy link

vercel bot commented Oct 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/srevinsaju/zap/Bqg1RmdaGdaFrRoQ8RqLhrUb48iy
✅ Preview: https://zap-git-fork-hadihammurabi-command-shorthand-srevinsaju.vercel.app

appimage.go Outdated Show resolved Hide resolved
@srevinsaju
Copy link
Owner

Looks really neat! Thank you for your patch! 🎉

@hadihammurabi
Copy link
Author

hadihammurabi commented Oct 5, 2021

I have fix things to change.
Please check

@@ -12,7 +14,7 @@ import (

func installAppImageOptionsFromCLIContext(context *cli.Context) (types.InstallOptions, error) {
executable := context.String("executable")
appName := context.Args().First()
context, appName := parseSourceFromAppName(context, context.Args().First())
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to calculate the appName from the parameter too, like

zap install repo/software1

should automatically set the appName = software1

and for local files

zap install ./super-cool-x86_64.AppImage

it should set appName = super-cool

@@ -74,7 +74,7 @@ func parseSourceFromAppName(context *cli.Context, appName string) (*cli.Context,
filePath := "file://" + appName
context.Set("from", filePath)
splitted := strings.Split(appName, "/")
return context, splitted[len(splitted)-1]
return context, strings.ReplaceAll(splitted[len(splitted)-1], "-x86_64.AppImage", "")
Copy link
Owner

Choose a reason for hiding this comment

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

Haha, that actually won't really help if the user's architecture is 32-bit for example.

Copy link
Owner

Choose a reason for hiding this comment

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

in case if the user's architecture is 32 bit, the file name will be someapp-i386.AppImage or someapp-i686.AppImage. If the user is using an arm 32, it will be someapp-armhf.AppImage or someapp-arm64.AppImage. There are certain apps which have this format someapp-4233cfe-x86_64.AppImage where the 4233cfe is the version of the app. My best guess is that, probably you should:

  1. split the filename by -
  2. take the first part of it
  3. replace _ by - and set appname as the result of the change.

@srevinsaju
Copy link
Owner

@hadihammurabi are you still working on this, or shall I hand it over to @kadern0 ?

@hadihammurabi
Copy link
Author

Yes, i still collecting cpu architectures types.

@srevinsaju
Copy link
Owner

@hadihammurabi it's there in the zap source code, https://github.com/srevinsaju/zap/blob/main/internal/helpers/arch.go, gives a list of supported architectures, but my proposed solution was more simpler than needing the list of architectures, but if it helps 🙂

@srevinsaju
Copy link
Owner

@hadihammurabi ping

@srevinsaju srevinsaju mentioned this pull request Oct 8, 2022
7 tasks
@hadihammurabi hadihammurabi closed this by deleting the head repository Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants