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

Add capability for ini file usage from the command line #432

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brockn8r
Copy link

@brockn8r brockn8r commented Nov 10, 2023

A couple notes:

  1. You will notice that I assumed the path to the config file was at the location from which generate_interactive_bom.py is called.
  2. I commented out args bom_dest_dir and open_browser because I didn't want any opportunity for error by others forgetting to set those parameters in the ini file.
  3. This mod should address issue Using local config file with CLI mode #368

This works mostly for us. We run on Linux (Fedora). I say mostly because there are still some oddities. For example, other than bom_dest_dir and open_browser, I still pass another option (--extra-data-file) on the command line even though I am using the ini file because I haven't figured out why it wasn't working.

@qu1ck
Copy link
Member

qu1ck commented Nov 10, 2023

Looks good, 2 general comments:

  1. You need to initialize the parser with defaults from class variables. You can not assume that ini files will have all values.
  2. Use appropriate getters (getbool, getint)

You will notice that I assumed the path to the config file was at the location from which generate_interactive_bom.py is called.

Actually you are checking locations where the board file is or where the plugin is. Which is correct.

I commented out args bom_dest_dir and open_browser because I didn't want any opportunity for error by others forgetting to set those parameters in the ini file.

That's why you need to use defaults.

I still pass another option (--extra-data-file)

It is intentionally not saved to the ini file because it is supposed to be dynamically chosen (unless explicitly specified by user).

@Funkenjaeger
Copy link
Contributor

Funkenjaeger commented Dec 1, 2023

I took a stab at addressing the two things @qu1ck pointed out in 49b31b2
I didn't create my own PR, assuming that @brockn8r will be back and can leverage my commit to finish this one.

I only did the most cursory testing, so I wouldn't yet assume it's bug-free...

@amauragis
Copy link

I think its unlikely that @brockn8r will be back to work on this (he is no longer in the role he was when creating it). If @Funkenjaeger wants to take ownership of the PR, great! Otherwise I'm happy to make a new one with those two commits....

@Funkenjaeger
Copy link
Contributor

@amauragis be my guest! I'm distracted with other projects lately, so it'd probably be a while yet until I focused on this enough to get it over the line.

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.

4 participants