your-tools / tbump

Bump software releases
BSD 3-Clause "New" or "Revised" License
158 stars 22 forks source link

Crash if `search` contains invalid substitution #86

Open dmerejkowsky opened 3 years ago

dmerejkowsky commented 3 years ago

Steps to reproduce:

[[file]]
src = "..."
search = "{no-such-key}"

tbump crashes with:

:: Bumping from 2.5.1-1 to 2.5.2-1
Traceback (most recent call last):
  File "/mnt/data/dmerej/.local/bin/tbump", line 5, in <module>
    module.main()
  File "/mnt/data/dmerej/src/tanker/tbump/tbump/main.py", line 183, in main
    run(args)
  File "/mnt/data/dmerej/src/tanker/tbump/tbump/main.py", line 103, in run
    bump(bump_options, operations)
  File "/mnt/data/dmerej/src/tanker/tbump/tbump/main.py", line 137, in bump
    executor = Executor(new_version, file_bumper)
  File "/mnt/data/dmerej/src/tanker/tbump/tbump/executor.py", line 50, in __init__
    file_bumper.get_patches(new_version),
  File "/mnt/data/dmerej/src/tanker/tbump/tbump/file_bumper.py", line 198, in get_patches
    change_requests = self.compute_change_requests()
  File "/mnt/data/dmerej/src/tanker/tbump/tbump/file_bumper.py", line 241, in compute_change_requests
    change_request = self.compute_change_request_for_file(file)
  File "/mnt/data/dmerej/src/tanker/tbump/tbump/file_bumper.py", line 278, in compute_change_request_for_file
    to_search = file.search.format(current_version=re.escape(current_version))
KeyError: 'no-such-key
plannigan commented 2 years ago

Something I noticed was that currently there are lots of places where .format(...) is called. This makes it tricky to track down all the places where that is called and it is hard to know what replacement keys can be used.

What would you think if I approached this with a bit of a bigger change? My idea is to have a single class that is only in charge of formatting these templates. This class would know the keys & values that could be used and only needs to be given the specific format template. This would make it easy to handle these errors in a consistent way so that the CLI can display the message. It also would standardize the replacement keys that are available and make it easy to add new ones if needed. (Could also address #109 and make it easier to achieve #85.)

This would move a lot of things around since new_version and current_version are currently passed into a lot of different places. So I wouldn't want to dive into that if it would be something that would be unlikely to be merged. Thought?

dmerejkowsky commented 2 years ago

So I wouldn't want to dive into that if it would be something that would be unlikely to be merged.

I think having a separate class is a really good idea -go for it ! We have plenty of tests so it's unlikely we get regressions - and even then we'll be able to fix them :)