Skip to content

Commit ea995ea

Browse files
committed
Fix string-based API version comparison in select_ref_for_plugin
The API version comparison used string comparison on the str() of Version objects (producing '3.0.0.final0'), compared against registry strings like '3.0' or '4.0'. This worked by coincidence for current values but would break with multi-digit components: - str '10.0' < '4.0' is True (wrong: 10 > 4) - str '3.10' > '3.9' is False (wrong: 3.10 > 3.9) Use Version.from_string() to parse registry min/max_api_version strings and compare against the actual Version objects from api_versions_tuple (the list of supported plugin API versions). Add regression test that verifies API version 10.0 correctly matches a ref with min_api_version='4.0'.
1 parent 69431e3 commit ea995ea

3 files changed

Lines changed: 48 additions & 8 deletions

File tree

picard/plugin3/manager/registry.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -131,20 +131,20 @@ def select_ref_for_plugin(self, plugin):
131131
if not refs:
132132
return None
133133

134-
# Get current Picard API version as string (e.g., "3.0")
135-
current_api = '.'.join(map(str, api_versions_tuple[:2]))
134+
# Get latest supported Picard plugin API version
135+
current_api = api_versions_tuple[-1] if api_versions_tuple else Version.from_string('0.0')
136136

137137
# Find first compatible ref
138138
for ref in refs:
139139
min_api = ref.get('min_api_version')
140140
max_api = ref.get('max_api_version')
141141

142142
# Skip if below minimum
143-
if min_api and current_api < min_api:
143+
if min_api and current_api < Version.from_string(min_api):
144144
continue
145145

146146
# Skip if above maximum
147-
if max_api and current_api > max_api:
147+
if max_api and current_api > Version.from_string(max_api):
148148
continue
149149

150150
# Compatible ref found

test/plugins3/test_plugin.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@
2525
patch,
2626
)
2727

28+
from PyQt6.QtCore import (
29+
QCollator,
30+
QLocale,
31+
)
32+
2833
from test.picardtestcase import PicardTestCase
2934
from test.plugins3.helpers import (
3035
backend_add_and_commit,
@@ -255,6 +260,11 @@ def test_name_falls_back_to_plugin_id(self):
255260
self.assertEqual(plugin.name(), 'my-plugin')
256261

257262
def test_lt_compares_by_name(self):
263+
import picard.i18n
264+
265+
QLocale.setDefault(QLocale('en'))
266+
picard.i18n._qcollator = QCollator()
267+
258268
p1 = Plugin(Path('/tmp'), 'alpha')
259269
p1.manifest = Mock()
260270
p1.manifest.name_i18n.return_value = 'Alpha'

test/plugins3/test_versioning.py

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
_parse_version_safely,
3232
_strip_to_first_digit,
3333
)
34+
from picard.version import Version
3435

3536

3637
class TestStripToFirstDigit(PicardTestCase):
@@ -228,7 +229,7 @@ def test_versioning_scheme_no_tags_falls_back(self):
228229
plugin.refs = [{'name': 'main'}]
229230
self.assertEqual(mgr.select_ref_for_plugin(plugin), 'main')
230231

231-
@patch('picard.plugin3.manager.registry.api_versions_tuple', (3, 0))
232+
@patch('picard.plugin3.manager.registry.api_versions_tuple', [Version.from_string('3.0')])
232233
def test_no_versioning_uses_refs(self):
233234
mgr = PluginRegistryManager(Mock())
234235
plugin = Mock()
@@ -237,7 +238,7 @@ def test_no_versioning_uses_refs(self):
237238
plugin.refs = [{'name': 'main', 'min_api_version': '3.0'}]
238239
self.assertEqual(mgr.select_ref_for_plugin(plugin), 'main')
239240

240-
@patch('picard.plugin3.manager.registry.api_versions_tuple', (3, 0))
241+
@patch('picard.plugin3.manager.registry.api_versions_tuple', [Version.from_string('3.0')])
241242
def test_api_version_skips_incompatible_refs(self):
242243
mgr = PluginRegistryManager(Mock())
243244
plugin = Mock()
@@ -249,7 +250,7 @@ def test_api_version_skips_incompatible_refs(self):
249250
]
250251
self.assertEqual(mgr.select_ref_for_plugin(plugin), 'picard-v3')
251252

252-
@patch('picard.plugin3.manager.registry.api_versions_tuple', (5, 0))
253+
@patch('picard.plugin3.manager.registry.api_versions_tuple', [Version.from_string('5.0')])
253254
def test_api_version_above_max_skips(self):
254255
mgr = PluginRegistryManager(Mock())
255256
plugin = Mock()
@@ -269,7 +270,7 @@ def test_no_refs_returns_none(self):
269270
plugin.refs = []
270271
self.assertIsNone(mgr.select_ref_for_plugin(plugin))
271272

272-
@patch('picard.plugin3.manager.registry.api_versions_tuple', (2, 0))
273+
@patch('picard.plugin3.manager.registry.api_versions_tuple', [Version.from_string('2.0')])
273274
def test_no_compatible_ref_uses_first(self):
274275
mgr = PluginRegistryManager(Mock())
275276
plugin = Mock()
@@ -279,3 +280,32 @@ def test_no_compatible_ref_uses_first(self):
279280
{'name': 'main', 'min_api_version': '3.0'},
280281
]
281282
self.assertEqual(mgr.select_ref_for_plugin(plugin), 'main')
283+
284+
@patch('picard.plugin3.manager.registry.api_versions_tuple', [Version.from_string('10.0')])
285+
def test_api_version_multi_digit_comparison(self):
286+
"""Ensure API version 10.0 is correctly compared (not string-based)."""
287+
mgr = PluginRegistryManager(Mock())
288+
plugin = Mock()
289+
plugin.versioning_scheme = None
290+
plugin.git_url = 'url'
291+
plugin.refs = [
292+
{'name': 'main', 'min_api_version': '4.0'},
293+
]
294+
self.assertEqual(mgr.select_ref_for_plugin(plugin), 'main')
295+
296+
297+
class TestApiVersionComparison(PicardTestCase):
298+
"""Verify Version-based API version comparison works correctly."""
299+
300+
def test_simple(self):
301+
v = Version.from_string('3.0')
302+
self.assertEqual(v.major, 3)
303+
self.assertEqual(v.minor, 0)
304+
305+
def test_multi_digit(self):
306+
v = Version.from_string('10.0')
307+
self.assertEqual(v.major, 10)
308+
309+
def test_comparison(self):
310+
self.assertGreater(Version.from_string('10.0'), Version.from_string('4.0'))
311+
self.assertLess(Version.from_string('3.0'), Version.from_string('3.99'))

0 commit comments

Comments
 (0)