Skip to content

[add] series CLI - --sort-by argument for series show#1836

Merged
liiight merged 2 commits intodevelopfrom
cli-series-sort-by
May 26, 2017
Merged

[add] series CLI - --sort-by argument for series show#1836
liiight merged 2 commits intodevelopfrom
cli-series-sort-by

Conversation

@tubedogg
Copy link
Copy Markdown
Contributor

@tubedogg tubedogg commented May 23, 2017

Motivation for changes:

I want to sort by entity ID.

Detailed changes:

When using the series show CLI subcommand, you can now specify --sort-by identifier to sort by entity ID (aka season/episode) or --sort-by age for last seen date (default), and --descending or --ascending to specify the sort order.
get_all_entities now supports return entities sorted by identifier or age (default).
Renamed 'Entity ID' and 'Latest age' columns in series show output to 'Identifier' and 'Last seen', respectively.

When using the `series show` CLI subcommand, you can now specify
`--sort-by entity` to sort by entity ID or `--sort-by seen` for last
seen date (default), and `--descending` or `--ascending` to specify the
sort order.
help='Show the releases FlexGet has seen for a given series ')
show_parser = subparsers.add_parser('show', parents=[series_parser, table_parser],
help='Show the releases FlexGet has seen for a given series')
show_parser.add_argument('--sort-by', choices=('seen', 'entity'), default='seen',
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.

Maybe name the options age and entity_id?

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.

age is a good idea. I was trying to avoid using underscored suffixes in an attribute as I think it makes it easier to remember and use, but I can change it if you feel strongly about it.

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.

You're right about underscore. Is identifier too much of an internal lingo?

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.

I think it might be - plus the column is headed as "Entity ID" in the resulting table, so I think entity or entityid makes it simpler.

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.

Though I suppose "entity" is pretty internal, too. Maybe we change the column heading to "Identifier" and make the option the same?

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.

How about show-order?

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.

Any strong feelings on how to proceed? I think I like identifier or series best, and change the name of the column to "Identifier" or "Series ID", respectively.

show_order = show_parser.add_mutually_exclusive_group(required=False)
show_order.add_argument('--descending', dest='order', action='store_true', help='Sort in descending order')
show_order.add_argument('--ascending', dest='order', action='store_false', help='Sort in ascending order')

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.

I don't remember what happens if you don't use this group at all. What's the default?

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.

The default is False (ascending) as set in filter/series.py. But your comment got me thinking, if it's left off, what does options.order end up being? It worked fine in my testing though.

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.

store_true stores a default of False (and the inverse for store_false). If neither is specified on the command line, order is given the default value of the first add_argument call.

import argparse
parser = argparse.ArgumentParser()
parser.add_argument('--sort-by', choices=('age', 'entity'), default='age')
order = parser.add_mutually_exclusive_group(required=False)
order.add_argument('--ascending', dest='order', action='store_false')
order.add_argument('--descending', dest='order', action='store_true')

>>> parser.parse_args('--sort-by age'.split())
Namespace(order=True, sort_by='age')

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.

If i remember correctly, setting a store boolean type of action sets the default to the opposite action. So if your action is store_true the default will be False.
However, since you've got two action on the same dest, the last one added takes precedence, and in this case sets True as the default.

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.

The first one added takes precedence. I echoed it on the command line without specifying the argument and it was False. I then tried adding a third option, so the order was store_true, store_false, store_true, and it was still False.

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.

Cool, good to know

episodes = show_episodes(series, session=session)
seasons = show_seasons(series, session=session)
return sorted(episodes + seasons, key=lambda e: (e.first_seen or datetime.min, e.identifier))
if sort_by == 'entity':
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.

Maybe something like this:

    if sort_by == 'entity':
        key=lambda e: e.identifier
    else:
        key=lambda e: e.first_seen or datetime.min, e.identifier
    return sorted(episodes + seasons, key=key, reverse=reverse)

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.

Addressed in 811bc8a.

@tubedogg tubedogg changed the title [add] series CLI - Sort option for series show [add] series CLI - --sort-by argument for series show May 25, 2017
@liiight
Copy link
Copy Markdown
Member

liiight commented May 26, 2017

This is done right?

@tubedogg
Copy link
Copy Markdown
Contributor Author

Yes sir.

@liiight liiight merged commit dba8b0b into develop May 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants