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

BUGFIX: 'Connect' command tab completion does not list purchased servers #1905

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

Nerdpie
Copy link
Contributor

@Nerdpie Nerdpie commented Jan 7, 2025

Fixes #1904

Repro steps:

  1. Purchase a server
  2. Connect to a non-purchased server (e.g. connect n00dles
  3. Type connect and press Tab
  4. The hostname of the server from step 1 is shown in the tab completion list

Note: Format and lint have been run

@Nerdpie
Copy link
Contributor Author

Nerdpie commented Jan 7, 2025

... yeah, that tracks that I'd overlook an associated test case.

Let me know if this fix makes sense, and I can adjust the test to match.

Edit: Upon closer inspection, the test is failing because the autocomplete code is now including home, even though that is the current server connected.

One possible fix would be to add

.filter(server => server !== currServ)

into the chain, or to directly add that condition to the existing filter, though it would push the length out a bit.

I'm not sure what approach would be most desirable.

@d0sboots
Copy link
Collaborator

d0sboots commented Jan 8, 2025

I'm really torn about this whole concept.

On one hand, yes, the player-purchased servers are valid targets to connect to, from anywhere on the network.

On the other hand, with the exception of home, almost none of them are ever useful targets to connect to, so adding them to completion would bloat the list of useful possibilities. And it does it for every server in the network, making it harder to read the useful servers.

@d0sboots
Copy link
Collaborator

So, we've talked about this a lot on discord (mostly in relation to a related issue, the parity between sing API and the general ability to connect). I think my overall feeling is this:

  • singularity and connect should have the same capabilities.
  • connect and tab-completion do not necessarily need to be the same.

The reason for the latter is that tab-completion is a convenience for the user, to make their navigation more streamlined. In most cases, this argues for parity between what is allowed and what is exposed via tab. However, given the large number of purchased servers (all the purchased ones, plus all hashnet nodes), combined with the fact that you very rarely want to connect to them, I think it is best to leave tab completion as-is.

...or so I thought, until I captured this reference image from tab-completion on home:
image

Yes, it's big, but all the "irrelevant" nodes are at the end. If this property remains guaranteed, it is pretty easy to visually ignore them if you're using tab to view possible servers. And since they have a small number of prefixes ("d" for "darkweb", "h" for "hacknet" which also overlaps with "home", and one other for bought servers) they don't interfere with regular tab-completion much. This just goes to show: Always include action screenshots in your PR! :D

@d0sboots
Copy link
Collaborator

As to fixes for the test: I think the correct thing is to add the "server !== currServ" to the condition, even though it will make it more bulky. (Extra filters means extra passes through the list, which strikes me as inelegant.) We shouldn't offer home as an option on home.

That should make this both pass, and be good to merge.

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.

BUG: CLI Tab Completion Doesn't List Purchased Servers
2 participants