Skip to content

Conversation

@Jamstah
Copy link
Contributor

@Jamstah Jamstah commented Jun 19, 2025

No description provided.

Signed-off-by: James Hewitt-Thomas <james.hewitt@gmail.com>
Copy link
Owner

@thp thp left a comment

Choose a reason for hiding this comment

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

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)

return None
except ValueError:
return next((job for job in self.urlwatcher.jobs if job.get_location() == query), None)
return next((job for job in self.urlwatcher.jobs if query in [job.get_location(), job.name]), None)
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: Tuple instead of list for membership check:

Suggested change
return next((job for job in self.urlwatcher.jobs if query in [job.get_location(), job.name]), None)
return next((job for job in self.urlwatcher.jobs if query in (job.get_location(), job.name)), None)

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