-
Notifications
You must be signed in to change notification settings - Fork 580
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 the ability to load multiple memories from files on the command line #1877
base: main
Are you sure you want to change the base?
Conversation
@@ -321,9 +325,9 @@ public static void run(Startup args) { | |||
|
|||
// we load the ram before first propagation | |||
// so the first propagation emits correct values | |||
if (args.getLoadFile() != null) { | |||
for (Pair<File, String> p : args.getLoadMems()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final var
please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to use a final.
However, not sure about the var here since a pair is used and the code is much more readable by explicitly stating the type of the pair to avoid any confusion when calling getLeft()/getRight() later.
Of course since this is my first contribution to the code base, I will not argue any further if you disagree.
src/main/resources/resources/logisim/strings/gui/gui_de.properties
Outdated
Show resolved
Hide resolved
src/main/resources/resources/logisim/strings/gui/gui_fr.properties
Outdated
Show resolved
Hide resolved
src/main/resources/resources/logisim/strings/gui/gui_ja.properties
Outdated
Show resolved
Hide resolved
src/main/resources/resources/logisim/strings/gui/gui_nl.properties
Outdated
Show resolved
Hide resolved
src/main/resources/resources/logisim/strings/gui/gui_pl.properties
Outdated
Show resolved
Hide resolved
src/main/resources/resources/logisim/strings/gui/gui_pt.properties
Outdated
Show resolved
Hide resolved
src/main/resources/resources/logisim/strings/gui/gui_zh.properties
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! The approach looks good to me. However, I think documentation of this enhancement needs to be improved! Is the enhancement also reflected in the output of logisim-evolution --help
?
src/main/resources/resources/logisim/strings/gui/gui_fr.properties
Outdated
Show resolved
Hide resolved
/* File contains the data that should be loaded into a RAM/ROM with a label matching the String | ||
(if no label is provided, File is loaded into every RAM/ROM) */ | ||
private List<Pair<File, String>> memoriesToLoad = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be also reflected in the user hints and documentation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about missing to update the documentation.
Question: how shall I handle languages other than English?
While I am happy to do it for English and French as I am fluent in these languages, what about the rest? ChatGPT/Google Translate? Or is there a way to notify/mark the fact that a translation is no longer up to date? Will the pull request be on hold until all translations have been done?
I couldn't find any information about this in the "How to contribute" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I am happy to do it for English and French as I am fluent in these languages, what about the rest?
I'd say just update the languages you speak. Automated translation is usually fine as long as we have a proofreader, otherwise it can end up with nonsense esp. we talk technical language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @MarcinOrlowski, please take care of the languages you speak. In the other language versions, you may add a TODO marker (in English) drawing attention that it is outdated, but you don't have to proactively do automatic translation. The PR won't be put on hold due to missing translations.
@mbaillif may be able to give further suggestions regarding documentation, as he has contributed most improvements to it over the past years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but should be reviewed by a second person. I would like to see at least the English documentation updated to reflect the enhancement plus ideally TODO comments in outdated documentation files.
/* File contains the data that should be loaded into a RAM/ROM with a label matching the String | ||
(if no label is provided, File is loaded into every RAM/ROM) */ | ||
private List<Pair<File, String>> memoriesToLoad = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @MarcinOrlowski, please take care of the languages you speak. In the other language versions, you may add a TODO marker (in English) drawing attention that it is outdated, but you don't have to proactively do automatic translation. The PR won't be put on hold due to missing translations.
@mbaillif may be able to give further suggestions regarding documentation, as he has contributed most improvements to it over the past years.
Currently, it is only possible to load a single RAM in a circuit in the command line. Ideally, we should not limit the loading to a single RAM, and allow the loading of one or more memories (either a RAM or a ROM).
This pull request adds the ability to specify on the command line the "label" of the memory we wish to load and allow the load argument to appear multiple times if more than one memories need to be loaded.