Skip to content

Fix merger support recognition on tarantool 2.10+ #227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

Totktonada
Copy link
Member

@Totktonada Totktonada commented Oct 19, 2021

We cannot use just string comparison as before, because '2.10.0-<...>'
less than, say, '2.3' lexicographically. The module had to use the old
select implementation on tarantool 2.10 due to this problem.

Implemented more accurate checks, whether built-in and external
tuple-merger are supported. Say, it'll not enable the built-in merger on
tarantool 2.4.1, where the module has a known problem (see comments in
the code).

Fixed the problem of the same kind in a test case.

@Totktonada Totktonada force-pushed the Totktonada/fix-tarantool-2-10-determination branch from 68ebfdc to d32f401 Compare October 19, 2021 01:47
@Totktonada Totktonada marked this pull request as draft October 19, 2021 01:48
@Totktonada Totktonada force-pushed the Totktonada/fix-tarantool-2-10-determination branch 3 times, most recently from f12b490 to 82a0518 Compare November 13, 2021 10:18
We cannot use just string comparison as before, because '2.10.0-<...>'
less than, say, '2.3' lexicographically. The module had to use the old
select implementation on tarantool 2.10 due to this problem.

Implemented more accurate checks, whether built-in and external
tuple-merger are supported. Say, it'll not enable the built-in merger on
tarantool 2.4.1, where the module has a known problem (see comments in
the code).

Fixed the problem of the same kind in a test case.

When I replaced `_G._TARANTOOL` with just `_TARANTOOL`, the luacheck
linter said me:

 | <...>/test/helper.lua:307:31: accessing undefined field match of
 | global _TARANTOOL

On the line:

 | local major_minor_patch = _TARANTOOL:split('-', 1)[1]

But when I added '_TARANTOOL' into globals, it stops to complain.
Strange, but okay. I prefer just `_TARANTOOL` without `_G`.
@Totktonada Totktonada marked this pull request as ready for review November 13, 2021 10:28
@Totktonada Totktonada requested a review from olegrok November 13, 2021 10:28
Copy link
Contributor

@olegrok olegrok left a comment

Choose a reason for hiding this comment

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

Thanks for your patch. I left several comments. If you don't want to fix version checks for "fieldpaths" and "uuids", please file an issue.

@Totktonada Totktonada merged commit fbe4de0 into master Nov 16, 2021
@Totktonada Totktonada deleted the Totktonada/fix-tarantool-2-10-determination branch November 16, 2021 07:16
Totktonada added a commit that referenced this pull request Nov 16, 2021
* Fixed the incorrect JSON path indexes check (it was introduced in
  2.1.2).
* Properly commented all checks.
* Correctly handle `tarantool-3*`, `tarantool-4*` and so on.
* Normalized as DNF for readability.

It follows changes I made in PR #227 for merger checks.
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