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 functionality behind --dic argument in CU CLI #228

Closed
wants to merge 2 commits into from

Conversation

LupusE
Copy link

@LupusE LupusE commented Aug 17, 2024

As someone wanted to use the --dict in CLI and got an error, the source shows the functions load_key_file and load_dict_file are not defined.
After some tests, I believe the functionality does not need to be an own function, it is easier to read and understand in one block while parsing the keys.

The if ... elif ... else construct is not very elegant, but works better for the different types of comparing as a match ... case. It is good to extend, if a .dic file (or single keys from a file) needs to be converted.

Copy link

You are welcome to add an entry to the CHANGELOG.md as well

Copy link

Built artifacts for commit 251c13d

Firmware

Client

@doegox
Copy link
Contributor

doegox commented Feb 7, 2025

hello, you need to merge your last 2 commits and force-push on the MR branch, else the venv files will still land into the repo history.
Something like

  • git reset HEAD^
  • git commit -a --amend
  • git push --force

@LupusE
Copy link
Author

LupusE commented Feb 7, 2025

Thanks for the hint. I've tried ...
Maybe I need to cancel the PR and restart. The issue came up because Github flagged my account a few month ago. At this time some repos gone wild.
I even wonder why this change was transmitted to the RRG repo. I wanted to upload this only to my fork.

I hope it is clean, now. Else I will do a full reset, tomorrow.

@doegox
Copy link
Contributor

doegox commented Feb 7, 2025

Yes the venv are now completely gone, thanks.
Now someone has to find time to fix the CI issues first 😅 , nothing to do with your changes.

@LupusE LupusE closed this by deleting the head repository Feb 12, 2025
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