Fixes #453 - Reversing behavior of Row.lpush/Row.rpush

[claudep]

Co-authored-by: chim chenpan@xiaomai5.com

[codecov[bot]]

Codecov Report

Merging #454 into master will decrease coverage by <.01%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #454      +/-   ##
==========================================
- Coverage   90.42%   90.42%   -0.01%     
==========================================
  Files          28       28              
  Lines        2591     2589       -2     
==========================================
- Hits         2343     2341       -2     
  Misses        248      248

Impacted Files	Coverage Δ
src/tablib/core.py	82.98% <100%> (ø)	:arrow_up:
tests/test_tablib.py	98.51% <100%> (-0.01%)	:arrow_down:
src/tablib/formats/_html.py	100% <0%> (ø)	:arrow_up:
src/tablib/formats/_csv.py	100% <0%> (ø)	:arrow_up:
src/tablib/formats/_yaml.py	100% <0%> (ø)	:arrow_up:
src/tablib/formats/_dbf.py	100% <0%> (ø)	:arrow_up:
src/tablib/formats/_latex.py	100% <0%> (ø)	:arrow_up:
src/tablib/formats/_jira.py	100% <0%> (ø)	:arrow_up:
src/tablib/formats/_xlsx.py	96.73% <0%> (ø)	:arrow_up:
src/tablib/formats/_rst.py	95.08% <0%> (ø)	:arrow_up:
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f7e39c1...3362a84. Read the comment docs.

[hugovk]

HISTORY.md correctly notes this as a breaking change, and it's definitely fixing a bug.

However, we of course don't know how many people are using this and relying on the broken behaviour.

The cautious thing to do would be to use SemVer and put (deprecation?) warnings in release 1.10, and then make the breaking change in release 2.0.

[claudep]

Providing a deprecation period is nice when the user can do something about it in its code, like using another path to avoid the warning. Here, what would we suggest to do to avoid the deprecation in 1.1.0?

[hugovk]

Some options:

1. Just fix it, release as breaking change in 1.1.

   • Not following SemVer
   • May break code where people are relying on current behaviour
   • Non-major bump may surprise people who expect no breaking changes

2. Just fix it, release as breaking change in 2.0.

   • Follows SemVer
   • May break code where people are relying on current behaviour
   • But a major bump indicates breaking changes, more likely to check release notes

3. Deprecation warning in 1.1. Fix in 2.0.

   • Users can fix it and silence the warning
   • But silencing specific warnings isn't always easy, especially if it comes from a dependency
   • Not always straightforward to silence warnings
   • Need to un-silence for 2.0?

4. Introduce new, fixed methods in 1.1. Deprecate old, broken ones in 1.1. Remove old, broken ones in 2.0.

   • Warns people immediately about the bug
   • They can start using the fixed ones
   • No particular rush to get 2.0 out

I favour number 4.

For example, the new ones could be l_push/r_push or left_push/right_push or prepend/append.

Or better, we should look to the stdlib and use consistent naming with existing data structures.

• Lists have l.append(x), and l.insert(0, x) (there's no prepend because it's inefficient.

• collections.deque has append and appendleft.

How about this?

[hugovk]

Hmm, a slight complication. The existing append just calls rpush and is therefore also now broken (and is fixed by this PR, so needs to be in the release notes):

    def append(self, value):
        self.rpush(value)

[claudep]

My feeling is that few people are using this Row API. And if they are, they should have noticed that reversed behavior. However, I didn't read any bug report about that, so at some point, if they didn't report it, then they are also partly to blame. That's why I still prefer 1. or 2. A drawback of changing the API is that it will be different from the Dataset object which also has lpush and rpush.

[hugovk]

Good point, Row also has a similar lpush_col and rpush_col.

How about we go for option 2?

And perhaps make a 1.1 release first where we can at least mention the upcoming change in the release notes?

[claudep]

That sounds like a good plan.