todo: warn new commits after review

[priyank-p]

with all the fixes and suggestions also added lint-fix command

[joyeecheung]

Oh, this reminds me of another TODO: check commits after the last CI run!

[apapirovski]

I would like to make sure we standardize around whether we use Prettier or not. I'm noticing a lot of formatting changes in some PRs and IMO it does make it a bit hard to follow the changes. The other reason is that if some people use it and others don't then we'll end up with a really messy history.

Personally, I'm not a big fan of Prettier because it tends to really bloat the line count and indent level. But it's not a hill I'm planning to die on lol so if the rest like it, let's implement it project-wide.

[Tiriel]

@apapirovski an npm run lint script is included in the package. It must be run before committing. I made the same mistake :D

[apapirovski]

@Tiriel That's not the issue, the eslint rules for this project are very minimal. These are a result of https://github.com/prettier/prettier.

[priyank-p]

@apapirovski @Tiriel i came to notice that the code is changed on save that because of my editor as it tried to format code on save. Either is because we don't have enough rules in .eslintrc or something with my editor i disable those setting for now. with the fixes requested i would update the style to normal.

[apapirovski]

@cPhost What editor do you use? I suspect you have something like prettier-atom or its equivalent for another editor: https://atom.io/packages/prettier-atom

[priyank-p]

@apapirovski it online c9 editor. BTW i also added npm run lint-fix to fix small linting errors so its easier.

[Tiriel]

@apapirovski my bad, didn't mean to offense.

Anyway, that can go with the broader discussion about linting rules in automation. But I'm with you on the necessity!

[apapirovski]

@cPhost Interesting. It seems to have some sort of a beautifier that isn't prettier but I can't find much info re: how to disable it. :(

@Tiriel No, no it's fine. I just want to figure this out because there are two PRs open right now both adjusting formatting quite substantially.

[apapirovski]

@cPhost Actually, looks like the option would be in Project > Code Formatters > Format Code on Save. Definitely better if it's disabled so that lines that aren't changed don't get re-formatted.

[priyank-p]

@apapirovski that is what i used and i think that what caused it! But i mean it also has option for using .eslintrc and maybe it is because we don't have a rule for it.

I agree that it bloats the code and make it lot harder to review. If lots of pr came like this i think we should go ahead and open and issue.

[apapirovski]

But i mean it also has option for using .eslintrc and maybe it is because we don't have a rule for it.

I think it's because eslint doesn't necessarily have an issue with this new formatting (and neither do I, to be clear). There's nothing inherently wrong with it, it's more that it introduces a lot of churn in the codebase.

[codecov-io]

Codecov Report

Merging #65 into master will increase coverage by 0.07%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   97.97%   98.05%   +0.07%     
==========================================
  Files          13       13              
  Lines         445      463      +18     
==========================================
+ Hits          436      454      +18     
  Misses          9        9

Impacted Files	Coverage Δ
lib/pr_checker.js	96.12% <100%> (+0.62%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 174aa93...d0fc7b7. Read the comment docs.

[priyank-p]

@apapirovski @joyeecheung fixed all the changes requested.

[priyank-p]

@joyeecheung hopefully this is good to go.

[priyank-p]

@joyeecheung done. Added stubs and i meant to clear logs after every it so don't have to create new logger every time. I got that idea for other PR so copy...paste and forgot to edit it. 😄

[priyank-p]

Just want to know whats the point for the status. Is is just for the tests?

edit: I also added (commit) in front of commit message just so it is clear when warning.

[apapirovski]

Right now? No specific point. Long-term it's going to allow us to do stuff like determine whether land-pr can run or whether we need user input first to address some of the issues, etc.

[joyeecheung]

@cPhost I like the idea of (commit), although for consistency with the error logs, maybe [COMMIT]? Also we need to sync with https://github.com/joyeecheung/node-core-utils/pull/69 on this.

[apapirovski]

I'm still not a big fan of any preface for the commits. I mean it's pretty clear they're commits, especially since our commits have specific format... Just makes the text longer. I could get behind [COMMIT] if it replaced [INFO] or [WARN]... (but I don't even know if that's feasible)

[priyank-p]

@joyeecheung the question is would this log be fine [WARN] [COMMIT]

Edit: or what @apapirovski said log as [COMMIT] commit The only reason i put it there if there are two or three commit it looks somewhat unorganized or unclear.

[joyeecheung]

@apapirovski @cPhost What about bullet points?

[WARN] Changes were pushed since the last review:
[WARN] - src: fix xxx
[WARN] - test: fix xxx

[priyank-p]

@joyeecheung i just need some clarification on

Also we need to sync with #69 on this.

do you mean log only last couple of commit to make less noise.

edit: if so i think do it in new pr as it would be more tests and only make this PR Huge.

[apapirovski]

@cPhost just that we want the format to be consistent between the two PRs. So we'll need to have that one also use bullet points.

[priyank-p]

@apapirovski @joyeecheung done now logs come out as

[WARN] Changes were pushed since the last review:
[WARN] - fixup: new changes requested
[WARN] - prchecker: new format