Sheet cannot be renamed to its own name

Issue #404 wontfix
John Yeung created an issue

You should be able to rename a sheet to its own existing name, but currently you can't:

>>> wb = openpyxl.Workbook()
>>> ws = wb.active
>>> ws.title
'Sheet'
>>> ws.title = 'Sheet'
>>> ws.title
'Sheet1'

I believe the straightforward and proper way to fix this is simply to add a test at the beginning of the title setter:

if value == self._title:
    return

Comments (11)

  1. CharlieC

    I'm not convinced that this is really an error. Why would you want to rename a sheet with its own name?

  2. John Yeung reporter

    Why would you want to actively prevent it? Especially when the fix is so simple?

    Imagine the new name is coming from user input. As long as that new name satisfies all of Excel's requirements (which the old name certainly does!), why shouldn't openpyxl accept it?

  3. CharlieC

    I think the issue is probably subtler. Imagine you wish to rename an existing sheet with the name of another existing sheet.

    ws1.title = 'A' ws2.title = 'B' ws1.title = 'B' ws2.title = 'A'

    This will leaved ws1 with the title 'B1' and ws2 with the title 'A' which I think is a little confusing.

    I think it's probably best not to attempt to automatically resolve conflicting worksheet titles but raise an Exception. This would make it easier to allow someone to rename a worksheet with its own name.

  4. John Yeung reporter

    I am 100% OK with not trying to automatically resolve name conflicts. Raising an exception is the more idiomatic thing to do in Python than silently picking a new name.

    That said, your ('A', 'B') -> ('B1', 'A') example represents the status quo, and I'm not actively proposing to change that. I really am just proposing to fix the very specific case of trying to set a sheet title to itself. I don't see how taking away automatic renaming has any bearing whatsoever on my proposed fix.

  5. CharlieC

    The example is there too illustrate the current confusing behaviour: depending upon the order the results will be different.

  6. John Yeung reporter

    I agree that the current (auto-rename) behavior is confusing. I just want to be clear that:

    (1) The current behavior in no way impedes the fix I am proposing. There is no reason anything else must be changed or fixed in order for my proposal to be enacted.

    (1a) If you believe that the current auto-rename behavior should be replaced by raising an exception, that's great, but it's a completely different proposal, and you should open a new issue if you want to address it. I would vote for it. I just don't want that to hijack my current proposal, nor delay my current proposal. (I really still think it is a bug that you can't rename a sheet to its own existing name.)

    (2) I think it can be argued that auto-rename when creating a new sheet is also a different issue than when renaming an existing sheet. I am less opposed to auto-rename for new sheets, particularly when you don't specify a name. Even if you do specify a name, the fact that you have to call a method (instead of just using assignment syntax) is at least a clue that something special may happen.

  7. CharlieC

    I don't want to argue semantics. I think the current behaviour is pretty braindead but not really a bug and certainly not the special case of renaming a single sheet. I'd be happy to see a PR that solves the real underlying issue: renaming has to give the responsibility for resolving conflicts to the user. As this would be a change in behaviour it won't be included in a patch release.

  8. John Yeung reporter

    I'm not arguing semantics. I'm arguing for the simple principle of least surprise. Both Python and Excel let you do x = x. Try it in Excel: Right-click in the tab's name; select Rename; then do some editing such that before you press Enter, the name is the same as what it was; and then press Enter. Excel gives no complaint, and of course accepts the name. At the end of the process, you've effectively executed a NOP. This is not surprising at all. It is what anyone would expect. There is no conflict for anyone (whether Excel or the user) to resolve.

    Let me put it this way:

    You are saying that auto-renaming upon assignment should be replaced by raising an exception. OK, fine. So let's imagine that we do that:

    >>> ws.title
    'Foo'
    >>> ws.title = 'Foo'
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    KeyError: Sheet 'Foo' already exists
    

    Is that really what you want? Yes, it's better than silently changing the title to 'Foo1'. But why should there even be an exception? There is no conflict to resolve.

    What is NOT surprising is this:

    >>> ws.title
    'Foo'
    >>> ws.title = 'Foo'
    >>> ws.title
    'Foo'
    

    So you see, your proposal and my proposal are actually completely orthogonal. What you are saying is: "openpyxl currently handles conflicts in a surprising way". What I am saying is: "the definition of 'conflict' is broken in openpyxl".

  9. CharlieC

    Discussed this with @ericgazoni and we agreed this isn't a valid use case. A PR with a better more general solution would be welcome.

  10. Log in to comment