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

Implement general configuarion for bear #502

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

samu698
Copy link
Contributor

@samu698 samu698 commented Jan 20, 2023

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 :)

libconfig will be responsible for handling all the configuration state
of bear
This functions unwrap to a variable if the result is contains the value
you want to unwrap
This commit removes all instaces were envp gets passed to functions.

The environment variables are available in the environ variable, and
don't need to be passed from function to function.

Also environ is standardized by POSIX unlike the envp argument on the
main function.
@rizsotto
Copy link
Owner

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 libresult library, 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 libsys library. 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
Copy link
Contributor Author

samu698 commented Jan 24, 2023

Thank you for the kind words about my C++ code 😊

But you are right the quality of my commit messages are not that good, I'll split this unwieldy PR into smaller chunks.

Anyway the libresult change was just for convenience and can be removed, the libsys one is needed: it allows to launch subcommands using only the configuration.

About the config interface it's better to prefix with citnames_ or intercept_ the options that have the same name, for example citnames_output_file intercept_output_file

So in the end the plan is:
PR for libsys
PR for making intercept configurable
PR for merging the configuration into one library
PR for removing the executions in bear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants