Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add file and directory report skeleton #116

Merged
merged 70 commits into from
Mar 1, 2024

Conversation

letFunny
Copy link
Collaborator

@letFunny letFunny commented Jan 16, 2024

  • Have you signed the CLA?

The report will be the main data source for chisel.db. This PR introduces the skeleton and the reporting functionality in the package extractor.


Tests for checking the final report after slicing have been intentionally omitted. Until we land #113, it is very difficult to inspect and change the slicer tests. On top of that right now the tests would be meaningless because they do not conform to the business logic of chisel.db. Because we are developing the feature iteratively in small PRs, I will add the rest of business logic in the next PR together with all the tests. At that point we can inspect them and now if the implementation is working or not and discuss corner cases.

@letFunny letFunny added the Priority Look at me first label Feb 7, 2024
Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice. Only left a few comments.

internal/fsutil/create.go Outdated Show resolved Hide resolved
internal/fsutil/create.go Outdated Show resolved Hide resolved
internal/fsutil/create.go Outdated Show resolved Hide resolved
internal/deb/extract.go Outdated Show resolved Hide resolved
Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you!

internal/deb/extract.go Outdated Show resolved Hide resolved
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. It's close, with mostly simpler stuff.

internal/deb/extract_test.go Show resolved Hide resolved
} else {
c.Assert(treeDumpFSCreator(creator, dir), DeepEquals, test.result)
}
// [fsutil.Create] does not return information about parent directories
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bracketed naming doesn't seem typical. I believe the typical convention in Go is to document types cleanly (foo.Bar), or has something changed in this regard recently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that you can use brackets for documentation links here: https://tip.golang.org/doc/comment#doclinks

internal/slicer/report.go Show resolved Hide resolved
internal/slicer/report.go Outdated Show resolved Hide resolved
internal/slicer/report.go Outdated Show resolved Hide resolved
internal/slicer/report_test.go Outdated Show resolved Hide resolved
internal/slicer/report_test.go Outdated Show resolved Hide resolved
internal/slicer/slicer_test.go Outdated Show resolved Hide resolved
internal/slicer/slicer_test.go Outdated Show resolved Hide resolved
internal/slicer/slicer_test.go Show resolved Hide resolved
internal/deb/extract_test.go Outdated Show resolved Hide resolved
@letFunny letFunny requested a review from niemeyer March 1, 2024 11:37
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@niemeyer niemeyer merged commit c66b0c0 into canonical:main Mar 1, 2024
14 checks passed
@letFunny letFunny deleted the chisel-db-report branch October 17, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority Look at me first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants