search

nblockchain / conventions

MIT License
1 stars 10 forks source link

Fix bodyParagraphLineMinLength rule #130

Closed webwarrior-ws closed 1 year ago

webwarrior-ws commented 1 year ago

Subceeding min length should never be reported for empty lines.

Fixes https://github.com/nblockchain/conventions/issues/119

Co-authored-by: Parham parhaamsaremi@gmail.com

knocte commented 1 year ago
knocte commented 1 year ago

@aarani please review

webwarrior-ws commented 1 year ago
  • "fix issue 119" is a really bad title for a commit msg, that's the kind of thing that should be a just footer note.

I can't think of short title that describes what is being fixed.

knocte commented 1 year ago

CI is still red.

I can't think of short title that describes what is being fixed.

Pair with parham 60 seconds in jitsi to give you some inspiration.

aarani commented 1 year ago

Isn't this kinda HACKY? what if there are two lines empty?

parhamsaremi commented 1 year ago

Isn't this kinda HACKY? what if there are two lines empty?

Is it allowed to have two lines empty? :thinking: I thought between body and footer should be at most two new lines?

aarani commented 1 year ago

Isn't this kinda HACKY? what if there are two lines empty?

Is it allowed to have two lines empty? 🤔 I thought between body and footer should be at most two new lines?

I don't know about the rules, I'm just reviewing the code @knocte needs to comment on this.

parhamsaremi commented 1 year ago

It seems I can't edit the PR's title but I suggest something like this: "Fix false positive test for bodyParagraphLineMinLength rule"

knocte commented 1 year ago

"Fix false positive test for bodyParagraphLineMinLength rule"

Test? This PR is not fixing a test.

parhamsaremi commented 1 year ago

"Fix false positive test for bodyParagraphLineMinLength rule"

Test? This PR is not fixing a test.

Hmm right then maybe: "Add and fix false positive test for bodyParagraphLineMinLength rule based on issue#119"

knocte commented 1 year ago

Is it allowed to have two lines empty? 🤔

This is the wrong question to ask. The right question to ask is: should it be disallowed? why? and if we disallow it, what rule should bring the error? I have the feeling that this bug can be fixed, indeed, with much cleaner code, as Afshin rightfully points out.

knocte commented 1 year ago

"Add and fix false positive test for bodyParagraphLineMinLength rule based on issue#119"

Concentrate on commit msgs first, as I was warning to Taras, and then we talk about PR title.

parhamsaremi commented 1 year ago

"Add and fix false positive test for bodyParagraphLineMinLength rule based on issue#119"

Concentrate on commit msgs first, as I was warning to Taras, and then we talk about PR title.

I improved them. Do you think they still have problems?

knocte commented 1 year ago

@aarani please re-review

knocte commented 1 year ago

"Add and fix false positive test for bodyParagraphLineMinLength rule based on issue#119"

Why violate DRY? You're already mentioning issue 119 in the "Fixes" line at the end. You have to concentrate on explaining what are you fixing.

In the end I've fixed your commits and PR description (and added more refactoring commits on top), to shorten this back&forth that was taking too long for such a low-priority task.

But please go into the branch and check the commits (1 by 1) and their commit messages, to see how it's done correctly for next time.

knocte commented 1 year ago

FYI I'm not merging this yet because I don't understand why PR's CI is red. @parhamsaremi can you look at it as low-prio task when you have time please. Thanks

knocte commented 1 year ago

@parhamsaremi I didn't tell you to fix it, I asked you to understand why. Now you fixed it and I can merge this, but you didn't explain what was happening, please explain.

parhamsaremi commented 1 year ago

@parhamsaremi I didn't tell you to fix it, I asked you to understand why. Now you fixed it and I can merge this, but you didn't explain what was happening, please explain.

I thought the goal was to fix the CI. My first instinct was to just push 1by1 again to see if it'll be fixed and it fixed it. I'll try to figure out the problem using the logs in actions.

knocte commented 1 year ago

I thought the goal was to fix the CI.

If my goal was to fix the CI I would have asked "please fix the CI", but I didn't say that, I said "I don't understand why PR's CI is red", therefore the way to look at it is first try to understand it.

parhamsaremi commented 1 year ago

I thought the goal was to fix the CI.

If my goal was to fix the CI I would have asked "please fix the CI", but I didn't say that, I said "I don't understand why PR's CI is red", therefore the way to look at it is first try to understand it.

The sanity check step that failed had these commits as the commits it was checking: 1843681ef4a4115510724956a4b6e5ab163bcd1b 9067c2e049ad46450bcf27b8a1701686b9e48232 4cc47ea8a147433dff7c7c4fdbad4df060c6ef3f 93e428ea160fe24b3e73bf4104b463b86ef48b1d aa419bc7b930d88cfe4110f01a59033337dc3e18

now the problem seems to be with the last commit (aa41). CI link. This commit's CI is triggered at 8:37 GMT +3:30.

After that, you've pushed another commit with the following hash: CI link. This commit's CI is triggered at 8:48 GMT +3:30 (11 minutes after the previous one). 2ccc084d8187913decfac4b7f893010612b5bfce

Now the CI status of aa41 is red and 2ccc is green, but for some reason, the PR CI was looking at the aa41 commit and was started at 8:39 GMT +3:30 (two minutes after aa41 commit). CI link

Not sure what's the github's algorithm for running PR CI but looks like when you pushed your second commit, PR CI did not run again. because the next CI that I see in the actions which is for this PR is this link which was triggered by me at 12:28 GMT +3:30 (about 4 hours after your CI). When I triggered the PR it has looked at the correct commit (2ccc) with green CI and the problem was gone.