postprocess: move --exclude-file checking to after comment transfer but before comment checking#1555
postprocess: move --exclude-file checking to after comment transfer but before comment checking#1555kkysen wants to merge 3 commits intokkysen/postprocess-gc-cachefrom
--exclude-file checking to after comment transfer but before comment checking#1555Conversation
… but before comment checking This lets us do the comment transfer and cache the LLM response, but not error when checking the comments. This keeps the cache more stable (e.g., otherwise once you add failing fns to an exclude file, you have to delete them from the cache, or else it'll be out of sync). Also, this lets us see how changes affect the cache and if it no longer needs to be excluded.
This should warn us if excluding a fn is no longer necessary so that we can stop.
thedataking
left a comment
thedataking
left a comment
There was a problem hiding this comment.
I think this goes against the principle of least surprise. Exclude files and denylists generally mean "don't even attempt the operation here" but this PR changes the semantics of the exclude file to "try operation but accept failure and notify if operation succeeded". I don't think we should make that change.
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
I think this goes against the principle of least surprise. Exclude files and denylists generally mean "don't even attempt the operation here" but this PR changes the semantics of the exclude file to "try operation but accept failure and notify if operation succeeded". I don't think we should make that change.
Hmm. I agree that this can definitely be surprising given the naming. What do you think about changing the exclude list to be like the xfail stuff we've used elsewhere a bunch (and documenting it more thoroughly)? Hopefully that would be clearer, and also let us have more useful behavior. You previously mentioned we might want to only exclude a function if it fails due to a certain reason, and xfail behavior would be much more conducive to that. If that doesn't seem useful, though, I can close this PR. We lose some nice behavior, but we can work around that manually.
2 participants
This moves
--exclude-filechecking to after comment transfer, but before comment checking. This lets us do the comment transfer and cache the LLM response, but not error when checking the comments. This keeps the cache more stable (e.g., otherwise, once you add failing fns to an exclude file, you have to delete them from the cache, or else it'll be out of sync). Also, this lets us see how changes affect the cache and if it no longer needs to be excluded. And if a fn is excluded, but the comment transfer actually succeeds, there will be a warning so that we can stop excluding it.