Add Cli option `group` in output

[rrd108]

Details at #5

Add a cli option to group results by rule or file

[David-Pena]

@rrd108 we agree we can not work on this feature until #63 is finished right? to avoid steping in each other toes

[rrd108]

Yes, you are right.

I can wait with the rules implmentation while you are working on this. So go ahead if you like.

[David-Pena]

I remember you told me about the yargs package for the cli. This group option should be a namespaced one right? like the apply & ignore?

[rrd108]

Yes. Like those.

[David-Pena]

Hi @rrd108

I wanted to update you on my progress with this issue. It's definitely gonna involve breaking changes because the report functions in all rules now won't be in charge of any console log.

Right now I have how would be the rulesReport.ts after this changes and the output structure would be like this:

per rule ⬇️

per file ⬇️

This is how the report function would look like for functionSize rule ⬇️

Some helper functions to handle the data structure in rulesReport ⬇️

The output comes from here ⬇️

[David-Pena]

There are a few important things missing:

• [X] add cli option
• [X] add new interface for offensesGroup object
• [X] update output messages from all rules As you can see in the pictures, there are a few rules where in the message field appears the fileName again, this is because for lots of rules the output is like a one liner ${filePath}:${lineNumber} ... but with the group option the filePath is at the top so it shouldnt appear again (it fills the screen a lot). Or in other cases, the message field will require more information as you noticed in #5

  Ah ok. I guess we should add some text to the number to make it more clear what is it.

• [ ] update all tests right now the report function in all rules returns the array of the offenses. Not the length as before, so all tests need expect(reportFunctionSize()).toBe(1); to become expect(reportFunctionSize().length).toBe(1); expect(mockConsoleLog).toHaveBeenLastCalledWith but with this new struct there is no immediate log output, ideas?

This is the state of this feature. Could you give me some feedback? @rrd108

I created a branch for this so you can check as needed https://github.com/rrd108/vue-mess-detector/tree/david/feature/group-report

[rrd108]

David, it seems to be awesome, it will be a significant improvement! :star2:

However I would not call it a braking change. Users communicate only with the cli and there is no breaking behaviour there. Yes the output is very different, but I hardly beleive there is any dependent project what deals with the output. So I think this will not be a new major (1.0) version.

Today I will launch the documentation site and we have to change the output links to our docs links #103

If you need some help in the missings things above, turn them into a separated issue and assign it to me.

[David-Pena]

David, it seems to be awesome, it will be a significant improvement! 🌟

Great 🎉

but I hardly beleive there is any dependent project what deals with the output. So I think this will not be a new major (1.0) version.

Thanks for the explanation, it makes more sense.

If you need some help in the missings things above, turn them into a separated issue and assign it to me.

I think I'll finish it in an hour or two, but If I get stuck I'll let you know 🫡

[David-Pena]

I just encountered an issue with the tests because this line is core of the tests: expect(mockConsoleLog).toHaveBeenLastCalledWith but with this new struct there is no immediate log output. Do you have any idea how we can fix/work around this?

[rrd108]

As the new approach returns instead of logging the tests should check the return value of the function

[David-Pena]

You mean only leave the green part of the test and remove the red ones?

[rrd108]

You can remove the red ones and add something like expect(reportvfornokey()).tobe({})

Perhaps it will not be an empty object. I am off my computer so I can not give you the exact thing.

[David-Pena]

You can remove the red ones and add something like expect(reportvfornokey()).tobe({})

I think I know what you mean.

Today is a holiday but before logging off I want to finish updating the output messages to show you. The tests Ill fix them later at night probably

[David-Pena]

Without the --group cli option ⬇️

With the --group=file cli option ⬇️

Output messages updated (plus some color changes)!

What do you guys think? @rrd108 @Whomy09

[rrd108]

Looks cool 🤩

[David-Pena]

Hi @rrd108

You can remove the red ones and add something like expect(reportvfornokey()).tobe({})

What do you think about this tests ⬇️

And in cases where multiple offenses of the rule (in this case props mutation & parent/instance) ⬇️

the object we push to offenses is basically the object in the check method from each rule ⬆️

For the latter example, maybe we can adhere for to the previous logic of checking the last ouput, in this case we can check only expect(reportImplicitParentChildCommunication()).toStrictEqual(offenses[offenses.length - 1])

Let me know if it makes sense? (I just did it with this rule to verify with you if its okay to move forward)

[rrd108]

This would be a proper test

const filename = 'props-mutation.vue'
checkImplicitParentChildCommunication(script, filename)
expect(reportImplicitParentChildCommunication()).toStrictEqual([{
    description: `👉 ${TEXT_WARN}Avoid implicit parent-child communication to maintain clear and predictable component behavior.${TEXT_RESET} See: https://vuejs.org/style-guide/rules-use-with-caution.html#implicit-parent-child-communication`,
    file: "props-mutation.vue",
    message: `line #7 ${BG_WARN}(todo)${BG_RESET} 🚨`,
    rule: `${TEXT_INFO}vue-caution ~ implicit parent-child communication${TEXT_RESET}`,
  },
])

[David-Pena]

The thing is, in the check method for each rule, the files array is a constant in the scope of the rule file, so every call or invoke of the report method is stacking the results.

I think I tried to reset the files array right before the report returns it's offenses and it didn't work but I'll try again

If you want I can update the branch with this test example and you can give me a hand with this part 😅

[rrd108]

Yes plase commit your code.

I will take a look. Test should run in isolation, they must not have any effect on other tests.

[David-Pena]

Yes plase commit your code.

I just pushed a commit to the branch https://github.com/rrd108/vue-mess-detector/tree/david/feature/group-report.

The sample tests are in implicitParentChildCommunication.test.ts

If you run yarn tests everything will fail and the console would be unmanageable so narrow the test in the package.json

I will take a look. Test should run in isolation, they must not have any effect on other tests.

It always seems weird to me when I had to increase the expected result of a test based on the previous test result, my bad I didn't raise my hand about it 😔

[rrd108]

Heh?

Now I am confused

The test passes for me.

BTW you can limit what test to run yarn test implicitParentChild

[rrd108]

Sorry, I see what you mean.

Without filtering out the other tests the output is crazy.

Ok, I will get back to you if I found some solution. Tomorrow is the earliest.

[David-Pena]

Sorry, I see what you mean.

I was afraid I was losing it haha 😅

Without filtering out the other tests the output is crazy.

Literally, I spent almost an hour trying to make it work because its not that intuitive the behavior 😔

Ok, I will get back to you if I found some solution. Tomorrow is the earliest.

Thanks 🤝 I will try to find a solution too, I'll report back if I achieve something.

[rrd108]

:smile:

It was easy to fix e94185ee869f95cb8ad0b01289b176bd105d3884

[David-Pena]

It was easy to fix https://github.com/rrd108/vue-mess-detector/commit/e94185ee869f95cb8ad0b01289b176bd105d3884

🤯 You won't believe

I had everything and nothing at the same time 😂

[David-Pena]

Thanks for the fix!!

I'll start migrating the all tests and I think by tonight this whole feature can move on the next stage

[rrd108]

Sounds great 🤩