Skip to content

toolkit/app-gen-toc.py: Remove updateDeviceConfig() function. #9

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

Open
wants to merge 1 commit into
base: work-v1.104.0
Choose a base branch
from

Conversation

dpgeorge
Copy link
Member

It doesn't do anything except give an error if the config file contains certain entries. There's no harm having these entries there, so just skip the validation altogether.

It doesn't do anything except give an error if the config file contains
certain entries.  There's no harm having these entries there, so just skip
the validation altogether.

Signed-off-by: Damien George <[email protected]>
@dpgeorge dpgeorge requested a review from iabdalkader March 27, 2025 06:24
@iabdalkader
Copy link
Collaborator

iabdalkader commented Mar 27, 2025

I was just trying to follow the original logic from Alif, which removed certain sub-sections from the miscellaneous section, and then wrote the file out again.

I didn't actually read what the original script did, so I missed that it removed those items. Could it be removing them because the SE can't handle them, or just to save space? If the SE can't handle them, it's probably best to leave them out somehow by removing them from the loaded config (if it's loaded at some point) or just leaving the error as it is. I've never actually tested running a config that includes these items so not sure if it works with them included.

@iabdalkader
Copy link
Collaborator

Apparently they're removed because the SE doesn't make any use of them,and they have no effect on the binary output, so it's probably fine to leave them or remove them if you like.

@dpgeorge
Copy link
Member Author

Apparently they're removed because the SE doesn't make any use of them,and they have no effect on the binary output, so it's probably fine to leave them or remove them if you like.

OK so the options are:

  1. Leave things as they are and continue to emit an error if the entries exist. The user must then remove them manually from the config file for things to work.
  2. Merge this PR as-is. If the entries exist it shouldn't really matter.
  3. Change this PR so that it removes the entries automatically if they exist and uses the modified JSON file. That's quite an involved change because we don't want to write out to the file (this tool should not change any input files, it should consider them read-only) so we'd either need to write out a temporary file or pass around the modified JSON to the other functions that use it.

I think option (2) is fine.

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