Elegant input command handling about terminusbrowser

[pbui]

You can further simplify @noyoshi's code:

cmd, *args = cmd.split()

Requires Python 3.

[noyoshi]

I think a better way of handling routeCommand would be doing something like

cmdList = cmd.split()
cmd = cmdList[0]
args = cmdList[1:]

then passing in cmd and args to the functions systemCommands, chanCommands, etc.
I think using split here is fine, because we want each thing in the list. But we should be doing checks to make sure we have the appropriate number of args each function is expecting to prevent the error.

[sambattalio]

I might be wrong, but I think for point 3 the .site might be deprecated. We recently changed the structure and I think that it has very little use anymore.

[Sipty]

I was actually planning on wrapping up the command handlers with pytest tests, before implementing the new cmd,args[] discussed above.

As part of testing, I will add cmd & args size checks, per module requirements.

Would you like me to make a new ticket for the tests @wtheisen?

I will tag the size checks for commands to this, as it directly solves the ticket.

And maybe a different one for the cmd/arg change, as it's an ideological change, more than anything?

I would appreciate a sanity check on the above proposed :)

[wtheisen]

I think it's fine to wrap it all as one big change, and all of those sound very helpful and useful! I think what @pbui posted earlier also makes a lot of sense to use. Full steam ahead I'd say these are all very good ideas!

[ginglis13]

@Sipty

Hey Chavdar, looking at this now.

Feeding b or t causes the program to crash because those are shorthand for commands board and thread. We haven't added in a check to src/Commands/ChanCommands.py to ensure that the full command has been entered (which is why the error is an index error when only one arg, b or t, is passed). So if just b is entered as a command, it will attempt to run board(uvm, cmd[1]), but cmd[1] does not exist. This needs to be fixed, good catch.

Per your bullet points:

1. Agreed that the command should be lowercase'd, probably in the routeCommand function of src/commandHandlerClass.py.

2. I'm not so sure on I follow how this one affects what we have in place currently. Could you maybe give an example of a command being handled in this way? I think it'd also be good to hear from @wtheisen and @sambattalio on this one if it changes the way we are handling commands. If it's more so a syntactical change, and not a large structure change, I'm for it.

3. It looks like we don't do a very consistent job of setting the site attribute for a given view, which would explain why it is returning None when you attempt to debug for it. I just did a quick check of this:

I added self.uvm.currFocusView.site = SITE.HACKERNEWS to the __init__ method of src/Frames/hackernews/indexFrame.py.

Then, I added log.warning(self.uvm.currFocusView.site) to routeCommand, and navigated to an HN thread. When I checked the log : [ WARNING]:commandHandlerClass.py #32 --- SITE.HACKERNEWS

Again, good catch, this is something that needs to be fixed :)

[Sipty]

Thank you @ginglis13, I didn't realize what cmd's payload was meant to be. Things make more sense now!

@sambattalio In another issue, there was talks if possibly keeping .site, hence why I wanted to use it... but seems like the idea is to be able to run all commands from anywhere?

In any case,@noyoshi 's approach is seriously beautiful and simply handles the currently existing problem, without limiting any functionality, so I'll go with it for now!

[Sipty]

Thank you for the swift and very detailed response!

The ticket isn't very focused... as you can tell.Thing is, the false t/b cmd handling feels like a symptom of a bigger issue, that I'd like to nip once and not worry about in the future.

About point 2: I was trying to understand the reason behind using split, rather than an array itterator.. as it seems like it can be a can of worms. Haven't tested any possible issues yet, but it feels like a non-pythonical way of approaching that.

About point 3: Shall I take it this will be fixed soon? If not, I can add them as well. Probably a different issue/commit should be made though.

[ginglis13]

@Sipty the command is passed in as a string, captured from the edit text of the command bar. If one were to iterate over the command using for c in cmd: , c will represent an individual character in the command, not a word, which I believe is the reason for using .split() in this case. Is this a correct interpretation of your suggestion?

edit: here's a log output. All warnings referencing line 31 of commandHandlerClass.py show that the 'command' is a string containing the command and its argument(s)

[wtheisen]

So the reason t and b crash by themselves is they try to run the thread and board command with an incomplete list of arguments. I think we're going to have to use split() regardless due to the way the data is set up. However in order to fix these crashes, and ones similar, we just need to add more complete error checking to the individual commands themselves. This should happen in the different site command files I think. Nice catch on the bug though I had forgot to check for these things. Maybe @ginglis13 can write some more tests to check the commands? Add a command success boolean we can check after a command is run. This helps testing and also would allow us to provide feedback to the user as the the validity of their command.

[Sipty]

Adding to list:

the command 'history reddit' crashes the program as well