Add recipe ls command #8

2021-07-21T09:02:04Z

decentral1se commented

2021-07-21 09:02:04 +00:00

This works and outputs a sorted list of apps 😀

Not sure how I should organise the helpers tho, any thoughts?

Also, what can be tested here? Can I mock the JSON to do offline tests?

This works and outputs a sorted list of apps 😀 Not sure how I should organise the helpers tho, any thoughts? Also, what can be tested here? Can I mock the JSON to do offline tests?

image.png

46 KiB

roxxers requested changes 2021-07-21 09:14:56 +00:00

roxxers left a comment

First-time contributor

Not much to review since most of it is good! Just one note, and a situation where you don't return an error where you should. So gunna label it as requesting a change for that reason but its a one line change 💙

cli/recipe.go Outdated

						
				@@ -0,0 +61,4 @@

					url := "https://apps.coopcloud.tech"

					apps := make(AppsJson)

					if err := getJson(url, &apps); err != nil {

						logrus.Fatal(err.Error())

roxxers commented

2021-07-21 09:08:40 +00:00

First-time contributor

Your function never returns this error. It is just logging it and exiting the program. You should change this to return nil, err. This can then be dealt with by the command itself.

Your function never returns this error. It is just logging it and exiting the program. You should change this to `return nil, err`. This can then be dealt with by the command itself.

decentral1se marked this conversation as resolved

cli/recipe.go Outdated

						
				@@ -0,0 +83,4 @@

					Action: func(c *cli.Context) error {

						apps, err := GetAppsJSON()

						if err != nil {

							logrus.Fatal(err.Error())

roxxers commented

2021-07-21 09:09:44 +00:00

First-time contributor

This will never be run because you never return an error, see comment on GetAppsJSON

This will never be run because you never return an error, see comment on `GetAppsJSON`

decentral1se marked this conversation as resolved

cli/recipe.go Outdated

						
				@@ -0,0 +87,4 @@

						}

						tableCol := []string{"Name", "Category", "Status"}

						table := createTable(tableCol)

						for _, name := range sortByAppName(apps) {

roxxers commented

2021-07-21 09:12:47 +00:00

First-time contributor

I am not totally against this approach. I worry about the speed of it but honestly might just say fuck it and use this as our baseline for sorting maps.

👍 1

roxxers marked this conversation as resolved

roxxers commented

2021-07-21 09:19:35 +00:00

First-time contributor

Also, what can be tested here? Can I mock the JSON to do offline tests?

I've seen rumblings on mocking an actual http call in a unit test for a mock one that returns what ever you want. No rush on that right now since this is early days.

> Also, what can be tested here? Can I mock the JSON to do offline tests? I've seen rumblings on mocking an actual http call in a unit test for a mock one that returns what ever you want. No rush on that right now since this is early days.

🚀 1

roxxers added the enhancement label 2021-07-21 09:26:42 +00:00

decentral1se force-pushed recipe-ls from 5f4d2b88f5 to 302ebcb394

2021-07-21 11:28:55 +00:00

Compare

decentral1se changed title from ~~WIP: add recipe ls command~~ to Add recipe ls command

2021-07-21 11:29:03 +00:00

decentral1se commented

2021-07-21 11:29:56 +00:00

Fixed that one-liner, rebased and will merge when CI is green 🚀

decentral1se merged commit 05ff163386 into main

2021-07-21 11:30:15 +00:00

decentral1se referenced this issue from a commit

2021-07-21 11:30:15 +00:00

Merge pull request 'Add recipe ls command' (#8) from recipe-ls into main

decentral1se deleted branch recipe-ls

2021-07-21 11:30:31 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: toolshed/abra#8