Skip to content

[change] Extend series list help message. (#1720)#1737

Merged
liiight merged 1 commit intoFlexget:developfrom
haarts:elaborate-series-output
Mar 21, 2017
Merged

[change] Extend series list help message. (#1720)#1737
liiight merged 1 commit intoFlexget:developfrom
haarts:elaborate-series-output

Conversation

@haarts
Copy link
Copy Markdown
Contributor

@haarts haarts commented Mar 15, 2017

Motivation for changes:

See #1720

Detailed changes:

  • Added 1 line of helpful text.

Addressed issues:

To Do:

When 'implementing' this I noticed that most of the CLI plugins don't return anything when no additional CLI arguments are given. For example flexget series just returns as does flexget archive. flexget regex crashes (because it uses a map). That's rather confusing (for me at least).
I believe it would be helpful to print the help page with a leader like 'no option selected'. Would you agree?

@liiight
Copy link
Copy Markdown
Member

liiight commented Mar 15, 2017

That message is more confusing than helpful imo. Also, I'd do this when the default series list return nothing, like what happened to you.

About what you said with cli, I agree with you and I imagine this could be done by forcing our subparser options to contain at least one option.

@haarts
Copy link
Copy Markdown
Contributor Author

haarts commented Mar 15, 2017

Would it help if I'd write smt like: 'Use flexget series all to view all known series.'?
I'll make showing it conditional.
If alright I'll make the other footer text conditional then too. It makes little sense to show it when displaying an empty list.

@liiight
Copy link
Copy Markdown
Member

liiight commented Mar 15, 2017

Cool

@haarts haarts force-pushed the elaborate-series-output branch from d8d8565 to 2b0d05f Compare March 16, 2017 08:01
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The python way is to do:

if not query.count():

Also, I think this should be before the table construction, since displaying an empty table is weird IMO.
And maybe add some thing like: "No results found. Try using flexget series all to view all known series" to make it more clear.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Excellent!
I was a bit reluctant to change too much but I considered doing just that. Will do.

@haarts
Copy link
Copy Markdown
Contributor Author

haarts commented Mar 20, 2017

@liiight Not sure if you saw the latest commit I pushed.

@liiight
Copy link
Copy Markdown
Member

liiight commented Mar 20, 2017

I'm not sure why this over-complication is needed. just check if not query.count, send a message to console and return...

@haarts
Copy link
Copy Markdown
Contributor Author

haarts commented Mar 20, 2017

I merely split the original function into two. The diff might look daunting but it's almost trivial. Reasoning behind doing so is the fact that the original function became/was quite long hampering (my) understanding. But you're the owner. If you prefer I'll revert it. No problem.

@liiight
Copy link
Copy Markdown
Member

liiight commented Mar 20, 2017

I agree that it's quite long but i don't think this is the right time (or way) to address that.

@haarts
Copy link
Copy Markdown
Contributor Author

haarts commented Mar 20, 2017

I'll amend.
Just out of curiosity; what would be the way to address it. In general terms of course.

@haarts haarts force-pushed the elaborate-series-output branch from 45401ea to cf1b363 Compare March 20, 2017 10:38
Both help messages are conditional. One will show on an empty result set the other then there actually is something to detail.
@haarts haarts force-pushed the elaborate-series-output branch from cf1b363 to 5b9329d Compare March 20, 2017 10:39
@liiight
Copy link
Copy Markdown
Member

liiight commented Mar 20, 2017

Try to make it a DRY as possible, but since this is a UI plugin, and one that holds a significant amount of data and data types, some cluster is bound to exist.

@haarts
Copy link
Copy Markdown
Contributor Author

haarts commented Mar 21, 2017

I'm unsure why CI failed. The log is blank?

@liiight liiight merged commit 152a867 into Flexget:develop Mar 21, 2017
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