rizsotto
commented
1 year ago Hey @samu698 , thanks for working on this! Hope your exams were big success. :)
The PR shows a good place to land, but became a bit too big. You've been touching many modules (62 files), probably with good additions, but as a reviewer I can't see how they are contribute to the end result. To give you examples for this:
- change the
libresultlibrary, by adding new methods. These methods deviate from what Rust result type has. I would like to understand what the benefits for these methods before they got into the codebase. (Probably would like to see some unit tests for them too.) Therefore I suggest to create a separate PR for this, where we can discuss the pros and cons. - changed the
libsyslibrary. Same thing here. Might be good what those changes, but need some explanation why they are needed.
And the main thing which this PR is proposing is to create a configuration type (shared between the sub-commands). I like the idea to have a shared configuration, but I noticed that this is also changing the user interface (the config file the users need to write). Can we split this into two? (Create one PR, which makes the citnames/source/Configuration.* files available for bear and intercept. And another PR, when we change what's in the configuration.)
I would also raise the issue to not leak the implementation details into the configuration. I think our users does not want to see citnames section in the configuration file. (To them its not relevant how the program works internally.) The users still think in the concept of the build system: what are the compilers to include or exclude, and what to include into output file. I see that you've been trying to create types to hold both the command line arguments and the config file content. This is good idea! But maybe we should not expose those types to the users. What do you think?
I would thank you again to work on this project, and spending time to fix these issues. I see that you are writing good quality code. You probably do better C++ than me. My request to you would be to spend more time to explain the changes you make. Can do it in comments you write into the code. Can do it with small PRs, where the explanation gets into the PR description. It might end up in the same place where your one big PR would be, but allow your readers (the person who approves the PR) to understand it better. This is hard to do, I know. It slows things down, it gets you out of the flow. My solution for this is to create a series of commits which I submit in separate PRs. (Not necessarily one commit per PR, but something that logically together. The number of files are changed is not more than 8 or 10.) Working this way, I can still finish the change, don't loose momentum in the work. But allows my reviewers to catch up gradually. And if they ask me to make changes, the discussion is more focused, and resolves much faster. After the successful merge, I can just rebase my branch and can submit the next PR. What do you think? Do you want to try to split up this PR into smaller ones?
samu698
This PR moves various configuration parameters into a single configuration library. This allows to more easily to remove the executions of intercept and citnames from Bear. I will update this message with the list of things to do to complete this PR.
PS: I had to study for some exams and I couldn't work on Bear, but now I'm back :)