core/commands: pin ls: display types by default #1138

vitorbaptista · 2015-04-25T17:42:08Z

Please bear with me as these are my first lines of Go code 😄

This fixes issue #1134

I tried following the convention on other files (for example, I named the struct RefKeyObject following the pattern on LsObject), but let me know if I got something wrong.

It follows the usage pattern @jbenet described in the issue:

vitor@clarisse:~/go/src/github.com/ipfs/go-ipfs$ cmd/ipfs/ipfs pin ls 
QmT78zSuBmuS4z925WZfrqQ1qHaJ56DQaTfyMUF7F8ff5o direct

vitor@clarisse:~/go/src/github.com/ipfs/go-ipfs$ cmd/ipfs/ipfs pin ls --type=indirect --count                          
QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ indirect 1
QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA indirect 1
QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 indirect 1
QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect 1
QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU indirect 1
QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt indirect 1

vitor@clarisse:~/go/src/github.com/ipfs/go-ipfs$ cmd/ipfs/ipfs pin ls --type=all
QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 recursive
Qmcqtw8FfrVSBaRmbWwHxt3AuySBhJLcvmFYi3Lbc4xnwj recursive
QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn recursive
QmT78zSuBmuS4z925WZfrqQ1qHaJ56DQaTfyMUF7F8ff5o direct
QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU recursive
QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA recursive
QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt recursive
QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ recursive
QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y recursive

I couldn't find a --quiet option on pin ls, though, so there might be something missing.

The golint complains about a few things in this file:

vitor@clarisse:~/go/src/github.com/ipfs/go-ipfs$ golint core/commands/pin.go 
core/commands/pin.go:13:5: exported var PinCmd should have comment or be unexported
core/commands/pin.go:25:6: exported type PinOutput should have comment or be unexported
core/commands/pin.go:260:6: exported type RefKeyObject should have comment or be unexported
core/commands/pin.go:265:6: exported type RefKeyList should have comment or be unexported

But as it's already complaining about other similar things, I'm assuming this is a known issue and you've chosen to ignore it.

If there's any problem with the code, please tell me and I'll be happy to fix it.

Congratulations on IPFS! It's an amazing project.

cryptix · 2015-04-25T17:50:41Z

Code changes LGTM 👍

This PR (obviously) changes the output of the ipfs tool and thus the shareness test t0081-repo-pinning.sh broke. Can you try to update that as well, maybe?

vitorbaptista · 2015-04-25T18:06:26Z

Wow! That was fast, specially for a Saturday 😄

@cryptix I was looking into that, but I'm not sure what's the correct fix. For example, this test:

expecting success: 
    ipfs refs "$MBLOCKHASH" >refsout &&
    ipfs refs -r "$HASH_WELCOME_DOCS" >>refsout &&
    ipfs pin ls -type=indirect >indirectpins &&
    test_sort_cmp refsout indirectpins
> diff -u refsout_sorted indirectpins_sorted
--- refsout_sorted  2015-04-25 17:45:50.481095349 +0000
+++ indirectpins_sorted 2015-04-25 17:45:50.482095299 +0000
@@ -1,10 +1,10 @@
-QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo
-QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ
-QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA
-QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7
-QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y
-QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg
-QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC
-QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU
-QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU
-QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt
+QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo indirect
+QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ indirect
+QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA indirect
+QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 indirect
+QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect
+QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg indirect
+QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC indirect
+QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU indirect
+QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU indirect
+QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt indirect

The issue is because we're comparing the output of ipfs refs with ipfs pin, which has changed and cause this failure. There are many ways to fix it, but mainly I could either:

Change ipfs refs output as well to match ipfs pin
Compare just that the keys are the same, ignoring their type

I feel the latter is the best way to go for now, am I right?

There's also other test cases, like:

expecting success: 
    cat directpinout >allpins &&
    cat rp_actual >>allpins &&
    cat indirectpins >>allpins &&
    cat allpins | sort | uniq >> allpins_uniq &&
    ipfs pin ls -type=all >actual_allpins &&
    test_sort_cmp allpins_uniq actual_allpins
> diff -u allpins_uniq_sorted actual_allpins_sorted
--- allpins_uniq_sorted 2015-04-25 17:45:50.635087632 +0000
+++ actual_allpins_sorted   2015-04-25 17:45:50.636087582 +0000
@@ -1,20 +1,14 @@
 QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo indirect
 QmQV9YGi9yGQZq3X2AXThF5pvKmQkZzDxt41k4LY6FqfM3 direct
-QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ indirect
 QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ recursive
-QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA indirect
 QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA recursive
 QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn recursive
-QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 indirect
 QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 recursive
-QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect
 QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y recursive
 Qma1WXbAqgH92pkE7VG6fzqc8dt5CK7FmAtmcnjb6H2ZTu recursive
 QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg indirect
 QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC indirect
 Qmcqtw8FfrVSBaRmbWwHxt3AuySBhJLcvmFYi3Lbc4xnwj recursive
-QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU indirect
 QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU recursive
 QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU indirect
-QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt indirect
 QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt recursive
not ok 26 - 'ipfs pin ls -type=all' is correct

That are more complicated. I'm not sure why the same hashes are returned with different types. Any ideas?

jbenet · 2015-04-25T18:38:25Z

Will take a deeper look later today but so far: ideally would add the --quiet / -q flag as other commands have. With quiet on, only the hash should be output (as it is now)

—
Sent from Mailbox

On Sat, Apr 25, 2015 at 11:06 AM, Vitor Baptista notifications@github.com
wrote:

Wow! That was fast, specially for a Saturday 😄
@cryptix I was looking into that, but I'm not sure what's the correct fix. For example, this test:

expecting success: 
  ipfs refs "$MBLOCKHASH" >refsout &&
  ipfs refs -r "$HASH_WELCOME_DOCS" >>refsout &&
  ipfs pin ls -type=indirect >indirectpins &&
  test_sort_cmp refsout indirectpins
> diff -u refsout_sorted indirectpins_sorted
--- refsout_sorted    2015-04-25 17:45:50.481095349 +0000
+++ indirectpins_sorted   2015-04-25 17:45:50.482095299 +0000
@@ -1,10 +1,10 @@
-QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo
-QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ
-QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA
-QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7
-QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y
-QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg
-QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC
-QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU
-QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU
-QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt
+QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo indirect
+QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ indirect
+QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA indirect
+QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 indirect
+QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect
+QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg indirect
+QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC indirect
+QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU indirect
+QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU indirect
+QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt indirect

The issue is because we're comparing the output of ipfs refs with ipfs pin, which has changed and cause this failure. There are many ways to fix it, but mainly I could either:

Change ipfs refs output as well to match ipfs pin
Compare just that the keys are the same, ignoring their type
I feel the latter is the best way to go for now, am I right?
There's also other test cases, like:

expecting success: 
  cat directpinout >allpins &&
  cat rp_actual >>allpins &&
  cat indirectpins >>allpins &&
  cat allpins | sort | uniq >> allpins_uniq &&
  ipfs pin ls -type=all >actual_allpins &&
  test_sort_cmp allpins_uniq actual_allpins
> diff -u allpins_uniq_sorted actual_allpins_sorted
--- allpins_uniq_sorted   2015-04-25 17:45:50.635087632 +0000
+++ actual_allpins_sorted 2015-04-25 17:45:50.636087582 +0000
@@ -1,20 +1,14 @@
 QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo indirect
 QmQV9YGi9yGQZq3X2AXThF5pvKmQkZzDxt41k4LY6FqfM3 direct
-QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ indirect
 QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ recursive
-QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA indirect
 QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA recursive
 QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn recursive
-QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 indirect
 QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 recursive
-QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect
 QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y recursive
 Qma1WXbAqgH92pkE7VG6fzqc8dt5CK7FmAtmcnjb6H2ZTu recursive
 QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg indirect
 QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC indirect
 Qmcqtw8FfrVSBaRmbWwHxt3AuySBhJLcvmFYi3Lbc4xnwj recursive
-QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU indirect
 QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU recursive
 QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU indirect
-QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt indirect
 QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt recursive
not ok 26 - 'ipfs pin ls -type=all' is correct

That are more complicated. I'm not sure why the same hashes are returned with different types. Any ideas?

Reply to this email directly or view it on GitHub:
#1138 (comment)

jbenet · 2015-04-25T18:38:35Z

And thanks very much for the PR! :)

—
Sent from Mailbox

On Sat, Apr 25, 2015 at 11:06 AM, Vitor Baptista notifications@github.com
wrote:

Wow! That was fast, specially for a Saturday 😄
@cryptix I was looking into that, but I'm not sure what's the correct fix. For example, this test:

expecting success: 
  ipfs refs "$MBLOCKHASH" >refsout &&
  ipfs refs -r "$HASH_WELCOME_DOCS" >>refsout &&
  ipfs pin ls -type=indirect >indirectpins &&
  test_sort_cmp refsout indirectpins
> diff -u refsout_sorted indirectpins_sorted
--- refsout_sorted    2015-04-25 17:45:50.481095349 +0000
+++ indirectpins_sorted   2015-04-25 17:45:50.482095299 +0000
@@ -1,10 +1,10 @@
-QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo
-QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ
-QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA
-QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7
-QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y
-QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg
-QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC
-QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU
-QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU
-QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt
+QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo indirect
+QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ indirect
+QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA indirect
+QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 indirect
+QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect
+QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg indirect
+QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC indirect
+QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU indirect
+QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU indirect
+QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt indirect

The issue is because we're comparing the output of ipfs refs with ipfs pin, which has changed and cause this failure. There are many ways to fix it, but mainly I could either:

Change ipfs refs output as well to match ipfs pin
Compare just that the keys are the same, ignoring their type
I feel the latter is the best way to go for now, am I right?
There's also other test cases, like:

expecting success: 
  cat directpinout >allpins &&
  cat rp_actual >>allpins &&
  cat indirectpins >>allpins &&
  cat allpins | sort | uniq >> allpins_uniq &&
  ipfs pin ls -type=all >actual_allpins &&
  test_sort_cmp allpins_uniq actual_allpins
> diff -u allpins_uniq_sorted actual_allpins_sorted
--- allpins_uniq_sorted   2015-04-25 17:45:50.635087632 +0000
+++ actual_allpins_sorted 2015-04-25 17:45:50.636087582 +0000
@@ -1,20 +1,14 @@
 QmQPFh7rJw8xCh18SyoUeutMBGhiN7DyNJpMDpaNr9kiEo indirect
 QmQV9YGi9yGQZq3X2AXThF5pvKmQkZzDxt41k4LY6FqfM3 direct
-QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ indirect
 QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ recursive
-QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA indirect
 QmUFtMrBHqdjTtbebsL6YGebvjShh3Jud1insUv12fEVdA recursive
 QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn recursive
-QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 indirect
 QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 recursive
-QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect
 QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y recursive
 Qma1WXbAqgH92pkE7VG6fzqc8dt5CK7FmAtmcnjb6H2ZTu recursive
 QmcaVM36sciFjeoFeXtFuXsyDk1Ag9zbH7Wn5kFnm5rVcg indirect
 QmckpYtw6YTyU17PsYgkY27gdofuVWVdstSyVVzABxxapC indirect
 Qmcqtw8FfrVSBaRmbWwHxt3AuySBhJLcvmFYi3Lbc4xnwj recursive
-QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU indirect
 QmeEqpsKrvdhuuuVsguHaVdJcPnnUHHZ5qEWjCHavYbNqU recursive
 QmemCbZMLWzW2YQktVxdfKkykZyYZySjUSTdwiX2WrF9bU indirect
-QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt indirect
 QmfE3nUohq2nEYwieF7YFnJF1VfiL4i3wDxkMq8aGUg8Mt recursive
not ok 26 - 'ipfs pin ls -type=all' is correct

That are more complicated. I'm not sure why the same hashes are returned with different types. Any ideas?

Reply to this email directly or view it on GitHub:
#1138 (comment)

whyrusleeping · 2015-04-26T08:46:16Z

Please bear with me as these are my first lines of Go code 😄

your Go code looks just fine to me! 👍

For the first test failure, having a -q option as @jbenet says should solve the issue.

for the second, It looks like ipfs pin ls -type=all will only output each pin once, outputting only one type per key. I'm not sure how to solve this one off the top of my head, it may require some fancy use of sed, or maybe some modifications to the go code. I'll take a better look when i've had some more sleep.

vitorbaptista · 2015-04-26T22:27:33Z

@jbenet Instead of adding a --quiet flag, I added a --verbose. My reasons were:

Usually, --quiet means no output at all (e.g. in wget, curl, sed and grep), so for me as a user having it meaning less output is a bit surprising (although it's the pattern used by other commands);
Adding a --silent instead allows me to keep the current behavior, which fixes the tests without me having to go and add --quiet to them

The dht command uses --verbose as well, so I'm not adding a new pattern. Anyway, I'm happy to change it to --quiet instead if you prefer.

whyrusleeping · 2015-04-26T22:28:45Z

👍 to verbose.

vitorbaptista · 2015-04-26T22:29:20Z

The build is mostly green, apart from a not ok 19 - 'ipfs daemon' can be killed test on Go 1.4 on Linux. It feels like an unrelated failure, though. Maybe it's a test that's known to fail sometimes?

whyrusleeping · 2015-04-26T23:05:08Z

yeah, we have a couple tests that fail occasionally, mostly due to port conflicts on travis

whyrusleeping · 2015-04-26T23:06:03Z

This looks good to me, ill wait on @jbenet's approval for the merge

jbenet · 2015-04-27T08:30:44Z

@vitorbaptista thanks so much for the PR!! 👏 👏 👏 👏

so for me as a user having it meaning less output is a bit surprising

we picked this up from git. git itself isn't consistent (opting for -q/--quiet some times and -v/--verbose others), but we want to be consistent. I want us to stick to quiet here, or change it everywhere (breaking change, so better be worth it). what quiet here means is: "the crucial output". (fwiw, docker and other tools use -q/--quiet this way too).

In the long run I want:

the default to be very clear to the user
to be possible to get only the crucial output. (and --verbose=false does not seem intuitive to me, as --verbose tends to be what users ask for when they want more output, not less.)
silencing fully to be possible (it is not possible today anywhere). it is possible we could use --silent or even --no-output for this.

I don't care for the word specifically, but I think --quiet is well established for this use case. It does not necessarily mean either {zero, crucial} output, as it varies among very widely used tools already. I'd keep this but am open to other words. We'd have to change it everywhere though, and that's not to be taken lightly.

The dht command uses --verbose as well, so I'm not adding a new pattern.

Yeah, this is a "give me way more output to debug things" type of request.

Whether this (the pin output) qualifies as "normal or verbose outputs ({,-v}" or "normal or crucial outputs {,-q}" is tricky, but looking at other commands like ipfs {add, ls, object *} i think "normal or crucial" (i.e. -q is more consistent.

Sorry to be pedantic about this, but this sort of UX really matters. consistency, clarity, and intuitiveness are all important. also important to consider not breaking userspace, which already uses -q for this.

vitorbaptista · 2015-04-27T22:17:21Z

@jbenet Big +1 to trying to keep the interface consistent. I've changed it to be --quiet instead and fixed the tests.

I don't like the way I fixed the tests. I would rather fix the expected output instead of simply changing everything to call pin ls --quiet instead, but the way the tests are written makes it somewhat difficult to do another way, and it's not too bad (although I should've added a test for the new output).

jbenet · 2015-04-27T22:39:50Z

@jbenet Big +1 to trying to keep the interface consistent. I've changed it to be --quiet instead and fixed the tests.

thanks!

I don't like the way I fixed the tests. I would rather fix the expected output instead of simply changing everything to call pin ls --quiet instead, but the way the tests are written makes it somewhat difficult to do another way, and it's not too bad (although I should've added a test for the new output).

Yeah, tests for the pin command should test the output with and without --quiet. tests for other commands can use ipfs pin ls --quiet just fine.

vitorbaptista · 2015-04-27T23:38:44Z

@jbenet So I rebased to the current upstream to avoid some merge conflicts. Also, I changed the tests to avoid using --quiet. To do this, I had to manually add the refs types as the tests are using the output from ipfs refs which isn't able to add those types itself (AFAIK).

Unfortunately, I was unable to make the last test test_expect_success "'ipfs pin ls --type=all --quiet' is correct" ' work without the --quiet. It seems ipfs pin ls outputs some hashes twice (as indirect and as recursive), which seems like the expected behaviour (given that the current test pipes the output to uniq). I had to keep it using --quiet.

Indirectly, it's testing both --quiet and default output of ipfs pin ls, but it's quite messy. I would like to, at least, redo the same ipfs pin ls --all test without --quiet as a overall test for the functionality, but I'm afraid I won't have much time to add it. I tried understanding why some hashes were printed twice and how I could add that test, but ran out of time. Hopefully the tests pass and this PR is still useful as it is.

jbenet · 2015-04-28T00:06:45Z

@jbenet So I rebased to the current upstream to avoid some merge conflicts. Also, I changed the tests to avoid using --quiet. To do this, I had to manually add the refs types as the tests are using the output from ipfs refs which isn't able to add those types itself (AFAIK).

👍

Indirectly, it's testing both --quiet and default output of ipfs pin ls, but it's quite messy. I would like to, at least, redo the same ipfs pin ls --all test without --quiet as a overall test for the functionality, but I'm afraid I won't have much time to add it. I tried understanding why some hashes were printed twice and how I could add that test, but ran out of time. Hopefully the tests pass and this PR is still useful as it is.

Since this is better than what's there, we could merge this now and do the rest in the future.

The test failuers are:

one iptb https://travis-ci.org/ipfs/go-ipfs/jobs/60301503#L2801 (cc @whyrusleeping :/ )
one intermittent (ok)
one seems to be this change set: https://travis-ci.org/ipfs/go-ipfs/jobs/60301505#L1923-L1930

vitorbaptista · 2015-04-28T10:32:18Z

@jbenet I've just fixed the tests. The issue was just a small difference between sed on Linux and on Mac. Should be good to go 👍

jbenet · 2015-04-28T17:49:46Z

@vitorbaptista sorry to bother with this, but we need all commits to pass the tests, because we want git bisect to work well to debug problems. you can run all tests on every commit locally with:

make test_all_commits

(take a look at what it does).

feel free to squash commits together -- we care more about atomic logical commits than historical ones.

If you want to get only the hashes (i.e. the previous behaviour), you can use the `--quiet` flag.

vitorbaptista · 2015-04-28T18:16:42Z

@jbenet Oh, sorry! I've just squashed them all. That's a great policy, btw. I'll adopt it on my own projects from now on.

core/commands: pin ls: display types by default

jbenet · 2015-04-28T18:54:04Z

thanks @vitorbaptista ! 👏

vitorbaptista · 2015-04-30T15:54:54Z

Thanks! I know that specially for small patches like this PR, it's easier to simply go ahead and write the changes yourself. It requires great patience to review the code (and set up easy-to-do issues on the first place), but it's a great way to foster a community. Given the number of contributions you have, you seem to be doing well.

I wish you luck. You're doing an amazing project!

jbenet · 2015-05-01T12:21:14Z

I know that specially for small patches like this PR, it's easier to simply go ahead and write the changes yourself. It requires great patience to review the code (and set up easy-to-do issues on the first place), but it's a great way to foster a community.

Thanks! yeah, we want to create a very approachable environment. some things aren't (like parts of our codebase), but we're trying to get better. we care to make the project open and fun for everyone :)

I wish you luck. You're doing an amazing project!

thanks again!!

jbenet added ready labels Apr 25, 2015

vitorbaptista force-pushed the 1134-display-types-on-pin-ls branch from 42d68c0 to f6268bf Compare April 27, 2015 22:13

vitorbaptista force-pushed the 1134-display-types-on-pin-ls branch 3 times, most recently from 9a14ab8 to a356d0b Compare April 27, 2015 23:35

vitorbaptista force-pushed the 1134-display-types-on-pin-ls branch from a356d0b to 3ae22f7 Compare April 28, 2015 10:12

vitorbaptista force-pushed the 1134-display-types-on-pin-ls branch from 3ae22f7 to d008e95 Compare April 28, 2015 18:13

core/commands: pin ls: display types by default

5d1a25b

If you want to get only the hashes (i.e. the previous behaviour), you can use the `--quiet` flag.

vitorbaptista force-pushed the 1134-display-types-on-pin-ls branch from d008e95 to 5d1a25b Compare April 28, 2015 18:15

jbenet mentioned this pull request Apr 28, 2015

Godeps maintenance #1156

Merged

6 tasks

jbenet added a commit that referenced this pull request Apr 28, 2015

Merge pull request #1138 from vitorbaptista/1134-display-types-on-pin-ls

1b51d9d

core/commands: pin ls: display types by default

jbenet merged commit 1b51d9d into ipfs:master Apr 28, 2015

jbenet added backlog and removed backlog labels Apr 28, 2015

jbenet mentioned this pull request Apr 28, 2015

ipfs pin ls should learn to display types. #1134

Closed

vitorbaptista deleted the 1134-display-types-on-pin-ls branch April 30, 2015 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/commands: pin ls: display types by default #1138

core/commands: pin ls: display types by default #1138

vitorbaptista commented Apr 25, 2015

cryptix commented Apr 25, 2015

vitorbaptista commented Apr 25, 2015

jbenet commented Apr 25, 2015

That are more complicated. I'm not sure why the same hashes are returned with different types. Any ideas?

jbenet commented Apr 25, 2015

That are more complicated. I'm not sure why the same hashes are returned with different types. Any ideas?

whyrusleeping commented Apr 26, 2015

vitorbaptista commented Apr 26, 2015

whyrusleeping commented Apr 26, 2015

vitorbaptista commented Apr 26, 2015

whyrusleeping commented Apr 26, 2015

whyrusleeping commented Apr 26, 2015

jbenet commented Apr 27, 2015

vitorbaptista commented Apr 27, 2015

jbenet commented Apr 27, 2015

vitorbaptista commented Apr 27, 2015

jbenet commented Apr 28, 2015

vitorbaptista commented Apr 28, 2015

jbenet commented Apr 28, 2015

vitorbaptista commented Apr 28, 2015

jbenet commented Apr 28, 2015

vitorbaptista commented Apr 30, 2015

jbenet commented May 1, 2015

core/commands: pin ls: display types by default #1138

core/commands: pin ls: display types by default #1138

Conversation

vitorbaptista commented Apr 25, 2015

cryptix commented Apr 25, 2015

vitorbaptista commented Apr 25, 2015

jbenet commented Apr 25, 2015

That are more complicated. I'm not sure why the same hashes are returned with different types. Any ideas?

jbenet commented Apr 25, 2015

That are more complicated. I'm not sure why the same hashes are returned with different types. Any ideas?

whyrusleeping commented Apr 26, 2015

vitorbaptista commented Apr 26, 2015

whyrusleeping commented Apr 26, 2015

vitorbaptista commented Apr 26, 2015

whyrusleeping commented Apr 26, 2015

whyrusleeping commented Apr 26, 2015

jbenet commented Apr 27, 2015

vitorbaptista commented Apr 27, 2015

jbenet commented Apr 27, 2015

vitorbaptista commented Apr 27, 2015

jbenet commented Apr 28, 2015

vitorbaptista commented Apr 28, 2015

jbenet commented Apr 28, 2015

vitorbaptista commented Apr 28, 2015

jbenet commented Apr 28, 2015

vitorbaptista commented Apr 30, 2015

jbenet commented May 1, 2015