Skip to content

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 requested a review from niemeyer January 19, 2024 12:26
@letFunny letFunny added the Priority Look at me first label Feb 7, 2024
Copy link
Contributor

@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.

Copy link
Contributor

@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!

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.

} 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

@letFunny letFunny requested a review from niemeyer February 27, 2024 10:57
@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
@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