merge master into v6.0.0 release branch

[alifeee]

No description provided.

[lavigne958]

I will wait for the fix for #1354 to be merged so I do this merge only once. if that's ok with you.

[lavigne958]

now we'll wait for #1362 🙃

[alifeee]

ah... I have just realised the "minor cleanup" changes in #1357 will have to be reverted when they are merged into 6.0.0

as they use update with swapped arguments 🙅‍♂️

oops!

(the tests should still work just will throw warnings)

[lavigne958]

yes right we need to update that back too.

I am working on merging master into feature/release_6_0_0 , I am facing some issues in the merge conflicts.
I need to resolve the changes in get and get_values and the recent changes in the get_records with the starting and ending index.

Once this is solved I should be able to merge master as ISO compatible, then we can introduce the simplification of the get_all_records method.

[lavigne958]

I am getting there, I noticed: we now in 6.0.0 return [[]] empty matrix in case of empty values, and not just [] empty list.

That's one case done ✔️

Question @alifeee we have a test: test_update_works_with_swapped_values_and_range
to me we don't need it anymore as we removed the warning as we changed the method signature, do you agree that we remove the test right ? 🤔

[alifeee]

I am working on merging master into feature/release_6_0_0 , I am facing some issues in the merge conflicts.
I need to resolve the changes in get and get_values and the recent changes in the get_records with the starting and ending index.

Yes, those are quite involved changes... I hope you are having fun. Hopefully the test suite helps avoid any bad feelings.

I am getting there, I noticed: we now in 6.0.0 return [[]] empty matrix in case of empty values, and not just [] empty list.

Interesting. This may break some code? Did you have to change any tests for this? What does the documentation say that we return? Anyway, this should probably go in the migration guide

Question @alifeee we have a test: test_update_works_with_swapped_values_and_range
to me we don't need it anymore as we removed the warning as we changed the method signature, do you agree that we remove the test right ? 🤔

We changed the method signature. However, I think we should keep the "swap if wrong way round", at least for a while. As discussed in #1336, it will help users migrate. And if we keep sending the warning then maybe they will change their code... It may be annoying for it to suddenly change, especially if we can so "easily" tell if a user has swapped args...

[lavigne958]

I am working on merging master into feature/release_6_0_0 , I am facing some issues in the merge conflicts.
I need to resolve the changes in get and get_values and the recent changes in the get_records with the starting and ending index.

Yes, those are quite involved changes... I hope you are having fun. Hopefully the test suite helps avoid any bad feelings.

Actually they do ! I based my choices and solved conflicts based on the test results.

I am getting there, I noticed: we now in 6.0.0 return [[]] empty matrix in case of empty values, and not just [] empty list.

Interesting. This may break some code? Did you have to change any tests for this? What does the documentation say that we return? Anyway, this should probably go in the migration guide

It might break the code yes, I did not but I think we don't have enough test for that special case.

I will check documentation, I don't think we mention anything on empty value range return.

Probably, I need to confirm that before we used to return an empty list and now it an empty matrix. Which is different in fact as the top list is NOT empty, it contains an empty list which make it 1 element long ! 😬

Question @alifeee we have a test: test_update_works_with_swapped_values_and_range
to me we don't need it anymore as we removed the warning as we changed the method signature, do you agree that we remove the test right ? 🤔

We changed the method signature. However, I think we should keep the "swap if wrong way round", at least for a while. As discussed in #1336, it will help users migrate. And if we keep sending the warning then maybe they will change their code... It may be annoying for it to suddenly change, especially if we can so "easily" tell if a user has swapped args...

But you mean we keep this test in release v5.X.Y ? Or in v6.X.Y ?

Agreed we can keep a test that checks if arguments have swapped or not.

Improvements coming up !

[alifeee]

But you mean we keep this test in release v5.X.Y ? Or in v6.X.Y ?

both, I think

[lavigne958]

in v5.x.y version it's already here, I'll add it in feature/release_6_0_0 soon

[alifeee]

closed by #1371