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

Already on GitHub? Sign in to your account

Redo my recent PRs that broke tests #26

wants to merge 6 commits into


None yet
2 participants

lewang commented Oct 21, 2012

  • Also includes some test refacoring.

Would you explain what's the benefit of this change?


lewang replied Oct 22, 2012

The tests never tested the case when we have to pad spaces at the end of a line to fill out a rectangle. I have added a testcase for this.

To elaborate, this change will keep the coordinates of the rectangle correct, when such padding happens. If we use buffer positions then the end isn't pushed further with space insertions.

I am not convinced.
The coordinates of the rectangle are not used in iedit-rect yet and I am not sure if it is worthy just for writing test cases. They are plenty of ways to test that case.
I prefer to keep iedit as light as possible.


lewang replied Oct 29, 2012

It's not for writing test cases. I found the bug. Then wrote the test case that proved the bug. Then fixed the bug. The bug broke rectangles.

Here is the loop that it goes through

(loop do (progn
                 (push (iedit-make-occurrence-overlay
                          (move-to-column beg-col t)
                          (move-to-column end-col t)
                 (forward-line 1))
            until (> (point) end))

So move-to-column is adding spaces, but keep comparing (point) with end. This can only
work if end was saved as a marker, and not as raw buffer index.


lewang replied Oct 29, 2012

For my customizations to hook into iedit, it's also useful to have correct and up to date rectangle corners.

I knew rectangle is broken.
"iedit-rectangle" just records the corners before iedit-rect started. Out of date is not a problem since is not used lately.


lewang replied Nov 4, 2012

I knew rectangle is broken.

And this fixes it. Is it broken in other ways that you know of?


lewang commented Jan 7, 2013

What are your thoughts on this PR? It's starting to diverge from master, should I rebase?

@ghost ghost assigned victorhge Jan 18, 2013


victorhge commented Jan 18, 2013

Sorry for the late replay.
Neither does this fix bring any value, nor any bad. I will merge it lately.

@victorhge victorhge closed this Jan 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment