fix caching of file-categorization logic #1044
Conversation
CONTRIBUTING.rst
Outdated
| @@ -163,6 +163,7 @@ their individual contributions. | |||
| * `Alex Stapleton <https://www.github.com/public>`_ | |||
| * `Alex Willmer <https://github.com/moreati>`_ (alex@moreati.org.uk) | |||
| * `Ben Peterson <https://github.com/killthrush>`_ (killthrush@hotmail.com_) | |||
| * `Buck Evan, copyright Google LLC <https://github.com/bukzor>`_ | |||
There was a problem hiding this comment.
From CONTRIBUTING.rst:
It's important to make sure that you own the rights to the work you are submitting. If it is done on work time, or you have a particularly onerous contract, make sure you've checked with your employer.
@DRMacIver can confirm, but I don't think we can accept a patch owned by (eg) Google without some documentation proving it's licensed.
There was a problem hiding this comment.
I think this is fine as long as @bukzor has filled out the necessary internal paperwork RE open source contributions, which we can take his word on.
There was a problem hiding this comment.
Never having worked at Google, I'll take your word for it! @bukzor, that means it's just the test and whitespace issue to go 😄
There was a problem hiding this comment.
I'm an agent of the company in this respect. I verify that I put my copyrighted work under your license just as your other contributors do.
Strictly FYI: Because this project is under MPL, it falls into the category of "no approval necessary" projects. Google only asks that I use my @google.com email for commits, and any contributor-copyright is labelled as "Google LLC".
There was a problem hiding this comment.
@Zac-HD: Github says you've requested changes, but the "view changes" button 404s for me. I don't think I can get out of this state without your help.
There was a problem hiding this comment.
Neat - it's always nice to hear about sensible IP policies!
I suspect rebasing broke the link; Github really seems to discourage it 😢 (or they just can't be bothered supporting it...). You've made all the changes I'd request now anyway ✨
|
This should really have a regression test, especially since it's obviously already regressed once. I can contribute one if you sketch how it would work. Or, pointing out an existing similar test would be enough too. |
It's never really regressed - this was created a few months ago when we added coverage-guided testing, and was broken in the same way for as long as it's existed. A test that checks something is in the cache after running would be possible, but IMO is on the wrong side of the power/fragility/risk tradeoff to be worth writing. |
|
I actually agree this should have a regression test (I think the only reason not to write regression tests is when it would be really difficult or expensive to do so, and even then I'm not happy). I'd meant to comment that on the original pull request - generally speaking the right place and time to write a regression test is before you fix the bug! - but forgot to. |
fixes #1041