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

Symlink support; Loockup into the PATH; Ability to hide the terminal name #3

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

Conversation

ASolomatin
Copy link

Hi. This commit allows to set the terminal in the configuration to something like x-terminal-emulator, but the terminal name will be taken from the actual executable, pointed to by the symlink. I also added the display-name config option which can hide the terminal name from the context menu.

Copy link
Owner

@mwahlroos mwahlroos left a comment

Choose a reason for hiding this comment

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

Hi, and thanks for your pull request.

Some of your additions look like they could be useful, and I could merge them after a few changes.

Overall, please see if you could split the path/symlink lookup changes and the new configuration option into separate commits. Pure style changes should also preferably be in separate commits.

src/nautiterm/open_terminal.py Outdated Show resolved Hide resolved
src/nautiterm/open_terminal.py Outdated Show resolved Hide resolved
src/nautiterm/open_terminal.py Outdated Show resolved Hide resolved
src/nautiterm/open_terminal.py Outdated Show resolved Hide resolved
src/nautiterm/open_terminal.py Outdated Show resolved Hide resolved
src/nautiterm/open_terminal.py Show resolved Hide resolved
src/nautiterm/open_terminal.py Outdated Show resolved Hide resolved
@ASolomatin
Copy link
Author

Hi. I reverted the original commit and did the same in a different way, with many small commits. After that I did a little refactoring; The configuration and terminal logic have been moved to separate classes. This will help later with new features. Finally, the code style is set up with pylint. A little later, I plan to add a linter to the CI pipeline (already in progress in another branch https://github.com/ASolomatin/Nautiterm/actions/runs/1778422318), as well as unit tests and code coverage.

Copy link
Owner

@mwahlroos mwahlroos left a comment

Choose a reason for hiding this comment

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

Hi again, and sorry I haven't been able to look into this in a while. I hope to look into it a bit more soon.

I'm not quite sure I agree with the multiple classes. So far the extension really is only doing a fairly simple thing so I kind of like the code being very simple and straightforward, too. The separate configuration class might be a good idea if you plan on adding new configuration options, though.

I didn't really have time to look at the other changes much yet, but there are a couple of things that popped up:

  • It'd probably be better to use os.path.realpath instead of manually resolving symlinks. That should do all the special cases right, and it's more readable.
  • Now that I think of it, it might be better if the name of the new boolean configuration option were display-terminal-name, show-terminal-name or something, especially in the configuration file itself. display-name is otherwise fine but to me it sounds like it might contain the "display name" of the terminal rather than a flag. A small change would make it a lot less ambiguous. This may sound like a bit of nitpicking but the names of configuration options tend to be for keeps so it's better to get them right once. What the variable is called in the code matters less.

In the meantime, take care!

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