Allow job reference by name#846
Conversation
thp
left a comment
There was a problem hiding this comment.
Should this also be mentioned in the documentation somewhere?
Seems like "by location or index" should be changed to "location, index or name":
% ag 'location or index'
docs/source/manpage.rst
77: delete job by location or index
80: enable job by location or index
83: delete job by location or index
86: change the location of an existing job by location or index
89: test filter output of job by location or index
92: test diff filter output of job by location or index (needs at least 2 snapshots)
lib/urlwatch/config.py
93: group.add_argument('--delete', metavar='JOB', help='delete job by location or index')
94: group.add_argument('--enable', metavar='JOB', help='enable job by location or index')
95: group.add_argument('--disable', metavar='JOB', help='disable job by location or index')
97: group.add_argument('--change-location', metavar=('JOB', 'NEW_LOCATION'), nargs=2, help='change the location of an existing job by location or index')
98: group.add_argument('--test-filter', metavar='JOB', help='test filter output of job by location or index')
100: help='test diff filter output of job by location or index (needs at least 2 snapshots)')
What in corner cases such as a name being a valid index (right now now we check int(query) and index <= 0), or a name matching a URL?
Would it make more sense to formalize this, so we can easily extend it in the future? Less magic and less chance for ambiguity (does the user intend to match on name, location or index?).
For example:
<integer>... A valid integer is always an index (like now)location=<url>...location:prefix matches the URL (preferred new way, documented)name=<name>...name:prefix matches the name<anthing else>... Interpreted as location match (for legacy compatibility, not documented)
There was a problem hiding this comment.
Thanks! It might be worth noting that an index will always win over a name (good!) in case someone names a job with a number that's also an index. Also, if the name is non-unique, or the name matches one of the job locations, the first item in the list "wins", but that's something that the user must take care of when specifying the list of jobs.
An additional safety measure would be to instead of finding the first match, find all of them:
all_matches = [job for job in self.urlwatcher.jobs if query in (job.get_location(), job.name)]Then, if len(all_matches) == 0, nothing was found. If len(all_matches) == 1, we have found a unique match and can continue with all_matches[0], if len(all_matches) > 1, then we can print an error (and abort), with all matched items and their index (because the user can then re-run the command with the index, which is always unique).
Something like (made-up output, might not be correct):
$ urlwatch --enable foo
Error: Multiple jobs match "foo", use the index instead:
Index: 3
Name: foo
Location: http://example.com/abc
Index: 8
Name: foo
Location: http://example.org/xyz
After which the user can do:
$ urlwatch --enable 8
...
(works successfully)
I'm not sure if this was an issue before already (are URLs unique?), but it's worth adding, what do you think?
|
Now stuck behind #870 but will rebase after that's sorted. I had a play and switched the logic around a little and added a method that gets one job or rage quits. I think it simplifies some of the later logic too. I looked at listing the matching jbos with indexes, but the indexes are dynamically generated when used, so you can't get the index for a specific job after they've been filtered. To get consistent indexes we could add an idx attribute to the jobs and populate it when loading, but I didn't fancy doing the work. |
Signed-off-by: James Hewitt-Thomas <james.hewitt@gmail.com>
| raise SystemExit(1) | ||
| return job.with_defaults(self.urlwatcher.config_storage.config) | ||
| if len(jobs) > 1: | ||
| print('Matched multiple jobs: {!r}'.format(id)) |
There was a problem hiding this comment.
Can/should this possibly also pretty-print the value of all matching jobs? Especially with many jobs and a long list, printing what actually matched is a good idea, as it might be tedious/complicated for the user to manually figure out why there are multiple matches.
I ran into such an issue last year with a different project, and patched it then.
Matched multiple jobs: foo
vs
Matched multiple jobs: foo
Job:
Job:
...
|
Without indexes? With this change: |
No description provided.