Skip to content

Change import#3

Open
ElMartes wants to merge 22 commits intomasterfrom
change_import
Open

Change import#3
ElMartes wants to merge 22 commits intomasterfrom
change_import

Conversation

@ElMartes
Copy link
Collaborator

Cleaner initialization of the metro module

@philsmt
Copy link
Owner

philsmt commented Jul 13, 2020

Thanks for this attempt to tackle the package import conundrum.

Before we go into the technical details, please rebase this work on top of the current master with #2 merged now to make the diff more manageable. Furthermore, please run flake8 on your code. It will complain about a few formatting choices. For convenience, I updated the util/flake8.sh script for the latest version and applied its newest complaints.

Finally, the non capitalized command line descriptions are on purpose to be in line with the builtin entries created by argparse itself.

@ElMartes
Copy link
Collaborator Author

Rebased onto the master branch.
I changed the style according to the flake8 complaints, but there are still some complaints with other files not altered in this branch.
As for the capitalized descriptions: I changed them because they are formulated as full sentences including a period at the end, but I now instead removed the period, in accordance with the usual argparse style.

@philsmt
Copy link
Owner

philsmt commented Apr 3, 2024

Got EXtra-metro working with a minor name and argument change related to the init function. Indeed this interface makes sense, I'll double-check later why I ever passed the args object directly. Any particular reason you wanted to rename init to init_metro here?

Could you please revert the various style changes not relevant for the functionality change? This will also make the diff clearer.

@ElMartes
Copy link
Collaborator Author

ElMartes commented Apr 8, 2024

I reverted the style changes and fixed some minor importing issue.
Also I renamed the main initialization routine back to "init", which can be called via, e.g., metro.init(args.core_mode, args.kiosk_mode, window_title).

Copy link
Owner

@philsmt philsmt left a comment

Choose a reason for hiding this comment

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

As you may have noticed, we've been working on getting metro-sci compatible with modern versions of Python and Qt in fix/forward-compat. This is fairly critical for us to continue supporting it on modern hardware.

I'd love to get this done finally and most of the testing on it already. As its history is unfortunately a bit wonky by mergers of masters into it, I did not manage to rebase it. while merging into it seems to work. I would ask you to squeeze the commits please to get rid of the duplicate commits.

self.destroyed.connect(_on_device_destroyed)

# for correct/full initialization set as visible first
self.setVisible(True)
Copy link
Owner

Choose a reason for hiding this comment

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

What specific problem does this solve? It seems not ideal to potentially pop up a window briefly, just to hide it immediately.

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