ljharb / repo-report

CLI to list all repos a user has access to, and report on their configuration in aggregate.
MIT License
24 stars 11 forks source link

Display percentages for the statistics #54

Closed CapriciousRebel closed 2 years ago

CapriciousRebel commented 2 years ago

@ljharb I have added the row to display the % of āœ” repositories for the --all flag. Could you please review it and let me know if I am going the right direction?

CapriciousRebel commented 2 years ago

Thanks for the comments, I'll resolve them all asap. The reason I only implemented it for the --all flag at the moment was that I wanted to ensure if I am going in the right direction

CapriciousRebel commented 2 years ago

@ljharb I have implemented all the requested changes, could you please give my PR another look and suggest more changes?

CapriciousRebel commented 2 years ago

I have multiple tests failing, and I think this particular error is the main reason:

  generateDetailTable,

  return a generated detail table

    āœ– should be deeply equivalent
    ------------------------------
      operator: deepEqual
      expected: |-
        '{"0":["name/project-eraser\\nname/guidelines-questionnaire\\nname/challenges-book\\nšŸ”’ name/microservice\\nname/responsive-design\\nname/media-upload-app","āœ–"],"options":{"chars":{"top":"ā”€","top-mid":"ā”¬","top-left":"ā”Œ","top-right":"ā”","bottom":"ā”€","bottom-mid":"ā”“","bottom-left":"ā””","bottom-right":"ā”˜","left":"ā”‚","left-mid":"ā”œ","mid":"ā”€","mid-mid":"ā”¼","right":"ā”‚","right-mid":"ā”¤","middle":"ā”‚"},"truncate":"ā€¦","colWidths":[],"colAligns":[],"style":{"padding-left":1,"padding-right":1,"head":["red"],"border":["grey"],"compact":false},"head":["Repository","Access\\nDefBranch"]},"length":1}'
      actual: |-
        '{"0":["Stats","0% (0/1)"],"1":["name/project-eraser\\nname/guidelines-questionnaire\\nname/challenges-book\\nšŸ”’ name/microservice\\nname/responsive-design\\nname/media-upload-app","āœ–"],"options":{"chars":{"top":"ā”€","top-mid":"ā”¬","top-left":"ā”Œ","top-right":"ā”","bottom":"ā”€","bottom-mid":"ā”“","bottom-left":"ā””","bottom-right":"ā”˜","left":"ā”‚","left-mid":"ā”œ","mid":"ā”€","mid-mid":"ā”¼","right":"ā”‚","right-mid":"ā”¤","middle":"ā”‚"},"truncate":"ā€¦","colWidths":[],"colAligns":[],"style":{"padding-left":1,"padding-right":1,"head":["red"],"border":["grey"],"compact":false},"head":["Repository","Access\\nDefBranch"]},"length":2}'
      at: Test.<anonymous> (/home/runner/work/repo-report/repo-report/test/utils.test.js:72:5)
      stack: |-
  Error: should be deeply equivalent
  at Test.assert [as _assert] (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:311:54)
  at Test.bound [as _assert] (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:96:32)
  at Test.tapeDeepEqual (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:552:10)
  at Test.bound [as deepEqual] (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:96:32)
  at Test.<anonymous> (/home/runner/work/repo-report/repo-report/test/utils.test.js:72:5)
  at Test.bound [as _cb] (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:96:32)
  at Test.run (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:114:31)
  at Test.bound [as run] (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:96:32)
  at Test._end (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:217:11)
  at Test.bound [as _end] (/home/runner/work/repo-report/repo-report/node_modules/tape/lib/test.js:96:32)

I read up on deeply-equivalent assertion, but I am unable to understand how to fix it in this case. Could you please help me out?

ljharb commented 2 years ago

I think you'll need to update the test fixtures in test/fixtures/fixtures.js - line 81.

CapriciousRebel commented 2 years ago

Okay, I'll look into it!

CapriciousRebel commented 2 years ago

@ljharb I have added the necessary fixtures for the table in tests and that fixes the failing tests. I will now do some testing locally on my system and see if it runs as expected. Once I am satisfied, I'll let you know and then you can test and merge :)

CapriciousRebel commented 2 years ago

@ljharb so I tested out locally and found two places where the Stats row is not needed:

  1. ls argument
  2. --actual option In both of these places, there is no 'goodness' defined so there are no checks or crosses in the table, so no stats are needed, hence I simply added an if statement to filter out the stats row in these cases.

Now I have completed testing and fixing the PR on my end, kindly test it and give me feedback from your end :)

CapriciousRebel commented 2 years ago

Committed the suggestion and rebased!

ljharb commented 2 years ago

Landed in 193e42c