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

Minor simplification of Python boolean #305

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions parse.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@
extensions = [ext for ext in extensions if ext not in targets]
print(f"Extensions selected : {extensions}")

include_pseudo = False
if "-pseudo" in sys.argv[1:]:
include_pseudo = True
include_pseudo = "-pseudo" in sys.argv[1:]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree to the concise approach being followed here to reduce the number of lines of codes. Given that we should focus on optimizing the codes as much as possible, but at the same time, we should not forget its maintainability and readabaility.

Following, "Explicit is better than Implicit": the core tenet of Python, the already existing codes explicitly communicates the default state for the inclusion of pseudo instructions as (False) and only sets it to True conditionally, making the flow of logic clearer. Readers can follow the intent more easily than inferring the meaning from a single line assignment.

Additionally, I am tagging the discussions for reference regarding pseudo instructions inclusion for more information as to why we are using flags for explicit behaviour : #286 and #289

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say I agree here. This isn't more explicit; it's just more verbose.

This is standard best practice. See this blog for example.

In the long term it would be much better to use argparse of course, but this is a pretty straightforward simplification.


instr_dict = create_inst_dict(extensions, include_pseudo)

Expand Down
Loading