Go: fix dataset check errors #20838
Go: fix dataset check errors #20838
Conversation
This way we don't have to remember to transform it at all call sites.
|
DCA shows that this gets rid of the dataset check errors 🎉 . There is an increase in the number of extraction errors, but that brings it back to the number from before the overlay PR was merged, so presumably they were being masked by the database inconsistencies. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes dataset check errors by centralizing path transformation logic within the FileLabelFor function. The issue was that path transformation wasn't consistently applied at all call sites where file labels were created. By moving srcarchive.TransformPath() into FileLabelFor, the transformation is now automatically applied whenever a file label is generated, eliminating the need to manually transform paths at each call site.
Key Changes:
- Moved path transformation logic from call sites into the
FileLabelFormethod itself - Updated all
FileLabelForcall sites to pass untransformed paths instead of pre-transformed paths - Simplified the
FileLabelmethod to pass the raw path directly toFileLabelFor
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go/extractor/trap/labels.go | Centralized path transformation by moving srcarchive.TransformPath() into FileLabelFor and removed it from FileLabel |
| go/extractor/extractor.go | Updated extractFileInfo to pass untransformed file parameter to FileLabelFor instead of the pre-transformed path variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return l.fileLabel | ||
| } | ||
|
|
||
| // FileLabelFor returns the label for the file for which the trap writer `tw` is associated |
There was a problem hiding this comment.
[nitpick] The comment describes FileLabelFor as returning "the label for the file for which the trap writer tw is associated", but this function is now a general utility that can be used with any file path, not just the one associated with the trap writer. Consider updating the comment to reflect that it takes an arbitrary path parameter and transforms it. For example: "FileLabelFor returns the label for a file at the given path (after applying path transformation)".
| // FileLabelFor returns the label for the file for which the trap writer `tw` is associated | |
| // FileLabelFor returns the label for a file at the given path (after applying path transformation). |
2 participants
These errors were accidentally introduced in #20623 . We can't reproduce them locally and we don't know why. The cause seems to be that the path transformer isn't used in one place where a label for a file is used. I have moved the use of the path transformer into the function that makes file labels so we don't have to remember to transform it at all call sites.
Note: this is mostly a reversion of this commit from that PR, and then a different way of achieving the same thing. It would have been better if I'd done an explicit revert commit and then put the
TransformPathcall in a different place. But I don't want to change it now, since then it's not totally clear that the DCA run is for the code that will be merged.