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

Running with -Werrror raises deprecation warning #451

Closed
gera opened this Issue Jun 30, 2017 · 12 comments

Comments

2 participants
@gera
Contributor

gera commented Jun 30, 2017

Attempting to add a worksheet via workbook.add_worksheet() generates a PendingDeprecationWarning. The API (ie, workbook.add_workskeet() and workbook.add_chartsheet()) should not pass the is_chartsheet kwarg to the internal method workbook._add_sheet().

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 30, 2017

Owner

Hi Devendra,

Can you add a small working program that demonstrates your issue.

John

Owner

jmcnamara commented Jun 30, 2017

Hi Devendra,

Can you add a small working program that demonstrates your issue.

John

@gera

This comment has been minimized.

Show comment
Hide comment
@gera

gera Jun 30, 2017

Contributor

Hi John,

This snippet shows the issue:

>>> import warnings
>>> warnings.simplefilter('error')
>>> import xlsxwriter
>>> import StringIO
>>> 
>>> workbook = xlsxwriter.Workbook(StringIO.StringIO(), {'in_memory': True})
>>> workbook.add_worksheet('Test')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/bc/venv/local/lib/python2.7/site-packages/xlsxwriter/workbook.py", line 180, in add_worksheet
    worksheet_class=worksheet_class)
  File "/home/bc/venv/local/lib/python2.7/site-packages/xlsxwriter/workbook.py", line 653, in _add_sheet
    PendingDeprecationWarning)
PendingDeprecationWarning: 'is_chartsheet' has been deprecated and may be removed in a future version. Use 'worksheet_class' to get the same result
>>>

I ran into this because I was running with -Werror. According to https://docs.python.org/2/library/warnings.html#default-warning-filters PendingDeprecationWarnings are ignored by default. However, there's no reason for the module to invoke an internal method in a way that would generate a PendingDeprecationWarning.

Cheers,
Gera.

Contributor

gera commented Jun 30, 2017

Hi John,

This snippet shows the issue:

>>> import warnings
>>> warnings.simplefilter('error')
>>> import xlsxwriter
>>> import StringIO
>>> 
>>> workbook = xlsxwriter.Workbook(StringIO.StringIO(), {'in_memory': True})
>>> workbook.add_worksheet('Test')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/bc/venv/local/lib/python2.7/site-packages/xlsxwriter/workbook.py", line 180, in add_worksheet
    worksheet_class=worksheet_class)
  File "/home/bc/venv/local/lib/python2.7/site-packages/xlsxwriter/workbook.py", line 653, in _add_sheet
    PendingDeprecationWarning)
PendingDeprecationWarning: 'is_chartsheet' has been deprecated and may be removed in a future version. Use 'worksheet_class' to get the same result
>>>

I ran into this because I was running with -Werror. According to https://docs.python.org/2/library/warnings.html#default-warning-filters PendingDeprecationWarnings are ignored by default. However, there's no reason for the module to invoke an internal method in a way that would generate a PendingDeprecationWarning.

Cheers,
Gera.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 30, 2017

Owner

Hi Gera,

Thanks for the update.

I still don't see that issue though:

Ok. I see it now.

The deprecation notice comes from this PR: #401 where the submitter says:

Further improvements can be to remove is_chartsheet argument in _add_sheet() deprecate for a while and remove later.

So I'll probably leave the deprecation warning there for a month or two, or until the next release (whichever comes later).

John

Owner

jmcnamara commented Jun 30, 2017

Hi Gera,

Thanks for the update.

I still don't see that issue though:

Ok. I see it now.

The deprecation notice comes from this PR: #401 where the submitter says:

Further improvements can be to remove is_chartsheet argument in _add_sheet() deprecate for a while and remove later.

So I'll probably leave the deprecation warning there for a month or two, or until the next release (whichever comes later).

John

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 30, 2017

Owner

I confirm that I can reproduce.

Owner

jmcnamara commented Jun 30, 2017

I confirm that I can reproduce.

@jmcnamara jmcnamara changed the title from Do not use deprecated kwarg for internal calls to _add_sheet to Running with -Werrror raises deprecation warning Jun 30, 2017

@gera

This comment has been minimized.

Show comment
Hide comment
@gera

gera Jun 30, 2017

Contributor

Ok, cheers. I can wait till the next release, although I was hoping for a point release just for this as that allows us to run our tests with -Werror.

Contributor

gera commented Jun 30, 2017

Ok, cheers. I can wait till the next release, although I was hoping for a point release just for this as that allows us to run our tests with -Werror.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 30, 2017

Owner

I was hoping for a point release just for this as that allows us to run our tests with -Werror.

Unfortunately, I can't do that. I wasn't keen on adding a deprecation warning on an internal method, or at all, but it was/is used by people subclassing the module even if it isn't a public interface. So it is only fair to add some sort of warning.

Maybe your can run your tests with reduced warnings around your XlsxWriter, for now.

I'll make a few comments on your PR and I'll use that when I deprecate the warning.

Owner

jmcnamara commented Jun 30, 2017

I was hoping for a point release just for this as that allows us to run our tests with -Werror.

Unfortunately, I can't do that. I wasn't keen on adding a deprecation warning on an internal method, or at all, but it was/is used by people subclassing the module even if it isn't a public interface. So it is only fair to add some sort of warning.

Maybe your can run your tests with reduced warnings around your XlsxWriter, for now.

I'll make a few comments on your PR and I'll use that when I deprecate the warning.

@gera

This comment has been minimized.

Show comment
Hide comment
@gera

gera Jun 30, 2017

Contributor

I wasn't keen on adding a deprecation warning on an internal method, or at all, but it was/is used by people subclassing the module even if it isn't a public interface. So it is only fair to add some sort of warning.

That's not what I meant though, and it's not what the patch does. The deprecation warning on the internal method stays. The patch just fixes the internal usage of that internal method, so that the public API doesn't throw up the warning.

To me, a deprecation warning when using a public API make sense only when there's a non-deprecated way of doing things.

Contributor

gera commented Jun 30, 2017

I wasn't keen on adding a deprecation warning on an internal method, or at all, but it was/is used by people subclassing the module even if it isn't a public interface. So it is only fair to add some sort of warning.

That's not what I meant though, and it's not what the patch does. The deprecation warning on the internal method stays. The patch just fixes the internal usage of that internal method, so that the public API doesn't throw up the warning.

To me, a deprecation warning when using a public API make sense only when there's a non-deprecated way of doing things.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 30, 2017

Owner

The patch just fixes the internal usage of that internal method, so that the public API doesn't throw up the warning.

Ah, true. I'll merge the PR.

Owner

jmcnamara commented Jun 30, 2017

The patch just fixes the internal usage of that internal method, so that the public API doesn't throw up the warning.

Ah, true. I'll merge the PR.

@jmcnamara jmcnamara added bug and removed enhancement labels Jun 30, 2017

@gera

This comment has been minimized.

Show comment
Hide comment
@gera

gera Jun 30, 2017

Contributor

I'm still hoping for a point release with this, but it's not a big deal to wait if one is coming soon(ish).

Cheers,
Gera.

Contributor

gera commented Jun 30, 2017

I'm still hoping for a point release with this, but it's not a big deal to wait if one is coming soon(ish).

Cheers,
Gera.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 30, 2017

Owner

I'm still hoping for a point release with this, but it's not a big deal to wait if one is coming soon(ish).

Yes. This needs a release. I'll do one over the weekend.

Thanks for the PR and apologies for being a bit slow on the uptake on this.

Owner

jmcnamara commented Jun 30, 2017

I'm still hoping for a point release with this, but it's not a big deal to wait if one is coming soon(ish).

Yes. This needs a release. I'll do one over the weekend.

Thanks for the PR and apologies for being a bit slow on the uptake on this.

@jmcnamara jmcnamara reopened this Jun 30, 2017

@gera

This comment has been minimized.

Show comment
Hide comment
@gera

gera Jun 30, 2017

Contributor

Great. Cheers for the quick turnaround.

Contributor

gera commented Jun 30, 2017

Great. Cheers for the quick turnaround.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 1, 2017

Owner

Fixed in version 0.9.8. Thanks.

Owner

jmcnamara commented Jul 1, 2017

Fixed in version 0.9.8. Thanks.

@jmcnamara jmcnamara closed this Jul 1, 2017

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