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

Prompting errors when it fails to load. #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HassanAli66
Copy link

Due to the inconvenience of the Error handling process of SicTools. I"ve made some modifications that prompts all errors found in a .asm file when the simulator fails to load the file other than just printing those errors in a command line. because, sometimes windows users for example run the simulator from just double clicking on sic.jar file. so, they can"t see the errors when they have ones because there is no cmd or a terminal running to show these errors.

@HassanAli66 HassanAli66 changed the title [GUI] Prompting errors when it fails to load. Prompting errors when it fails to load. Oct 25, 2022
Copy link
Owner

@jurem jurem left a 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. Nevertheless, we still want to fully support command line usage of assembler, so don"t want to have any windows being popped up in text mode. Special handling should be made for GUI and TUI mode.

Additionally, print relates to printing not constructing string. String construction should be refactored in a separate function. Finally, the call to this new function in loadAsm() should be made only once (the same string construction is currently performed twice). I would also suggest use of StringBuilder instead of String.

@jurem
Copy link
Owner

jurem commented Oct 26, 2022

For the other contribution - title change is unnecessary. All contributors are acknowledged through github dashboard, git log etc.

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