Add open link under terminal cursor - #2062
Conversation
|
You can add it to keybindings.py, stick it in the keys dict and test to see if the keybinding works and I'll check it after |
|
I added the bindings, after reloading the schemas this seems to work fine. I'll let you have a look. |
Davidy22
left a comment
Davidy22
left a comment
There was a problem hiding this comment.
Works for me and changes look mostly fine. Forgot to ask for a release note file, just do a make reno SLUG=[descriptive_label] and fill in the release note file like how it is in other PRs. Also a thing about the type hint
| if value: | ||
| return value | ||
|
|
||
| def get_link_under_terminal_cursor(self) -> Optional[str]: |
There was a problem hiding this comment.
Type hints would be a nice thing to do but probably should be consistent with the rest of the codebase and everything else isn't hinted at the current moment. Moving files onto hints could be a good bigger project though.
There was a problem hiding this comment.
Yes this would be very helpful! I actually added a lot more locally to navigate and understand the code more easily. I left it out as this wasn't in the scope of this PR.
For this part specifically, I saw that Tuple and Optional were already used line 275 (though now a quick look shows this might have been the only case of typing in this codebase).
In my opinion every bit of typing help, whether in making code more understandable, or improving type errors and catching early type errors, with pretty much no down side. The fact that it can be adopted incrementally is also something to take advantage of, as larger nice-to-have projects tend to always be pushed back.
Removed it anyway, just food for thought for later PRs.
There was a problem hiding this comment.
Actually I wouldn't mind opening a PR to start adding some more type hints to the codebase. Surely not everything, but a good chunk of the low hanging fruits just to get things started. If that's something you'd be interested in.
There was a problem hiding this comment.
Yeah that'd be nice, can take that if you're willing to do it. Just need the release note file generated by make reno SLUG=[descriptive_label] filled in and I'll merge
There was a problem hiding this comment.
Release notes are now there.
Noted for the typing PR :D
thibthib18
changed the title
|
Alright looks fine, merging |
2 participants
Add functionality to set a keyboard shortcut to open link under terminal cursor (similar to "ctrl click").
Following 2060
Status: current changes are enough to open links under cursor via a hardcoded shortcut.
Question: how to properly add a new user configurable shortcut?