Skip to content
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

Removed the vendorized packages #195

Closed
wants to merge 2 commits into from

Conversation

tommyanthony
Copy link
Contributor

I've removed the vendorized versions of xlwt, xlrd, openpyxl, ominjson, unicodecsv, and pyyaml and replaced them by installing them in setup.py.

If there's a reason this hasn't been done already, I'd love to know and see if there's anything we can do to update the vendorized versions to newer releases.

Edit: Ok, I now see it fails on python 3.2. Not sure how important that is, but if that's why this hasn't been done this can be closed I guess.

unicodecsv, and pyyaml.  These were replaced by installing them in
setup.py and updated to their most recent versions.
@iurisilvio
Copy link
Collaborator

I want something like this too, it should be done, but don't know the right way to do it. That is the reason it is not done. I don't want to enforce the installation of all these packages.

I'll maintain it open and will try to make it happen soon.

The Python 3.2 support definitely is not a good reason, we can drop the support for this version.

@tommyanthony
Copy link
Contributor Author

Having dependencies installed via pip is pretty normal, see pyspider's setup.py: https://github.com/binux/pyspider/blob/master/setup.py#L56

Also, I noticed the last time the PyPI version was updated was a year ago, do you know how to update the version installed by pip?

@iurisilvio
Copy link
Collaborator

I agree, but I'm looking for a solution with extras. I'm planning to release a 1.0 soon, but no fixed dates yet. This is one big change I want to do.

unicodecsv from the installation requirements for python 3 as it
defaults to the standard library csv module in python 3.  Made omnijson
an optional installation as it will try to be imported, but if not found
will default to using the standard json library included with python.
@tommyanthony
Copy link
Contributor Author

I updated the pull request, maybe you'll like this one more. I removed the requirement of installing omnijson, and made it only install unicodecsv if using python 2. I also removed python 3.2 from .travis.yml.

If tablib can import omnijson it will still use it, otherwise it will default to the standard json library.

@claudep
Copy link
Contributor

claudep commented Jul 15, 2015

Looks like this PR is in good shape, doesn't it? Iuri, any remaining issue?

@kennethreitz
Copy link
Contributor

The vendored packages will stay for now... they work, and they allow you to use this package without installing 200 dependencies.

There are many Python 3 ports within this codebase that I did myself, because these projects were abandoned at the time. Hopefully this is no longer the case, but this will require a closer investigation.

@claudep
Copy link
Contributor

claudep commented Feb 7, 2016

200 dependencies? Aren't we talking about ~6 here?
A closer investigation? Aren't you trusting your own test suite?
You are the maintainer, you decide, I just find a bit sad to just put away contributed work with almost no rationale.

@kennethreitz
Copy link
Contributor

@claudep don't worry, I'll likely accept this soon. Your work is very appreciated. Just needs some time to brew.

I'll keep the PR open for peace of mind :)

@kennethreitz kennethreitz reopened this Feb 7, 2016
@claudep
Copy link
Contributor

claudep commented Feb 7, 2016

To be clear, it's not my work at all (I probably won't dare to say that for my own work :-) ).
Sorry if I misunderstood that closing the PR was simply ditching this work. If it's not the case, it's good news!

@kennethreitz
Copy link
Contributor

@claudep can you review the failing tests?

@kennethreitz
Copy link
Contributor

er, @tommyanthony can you review the failing tests?

@claudep
Copy link
Contributor

claudep commented Feb 7, 2016

My two cents: for the first failure:

======================================================================
ERROR: test_book_export_no_exceptions (__main__.TablibTestCase)
Test that various exports don't error out.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_tablib.py", line 414, in test_book_export_no_exceptions
    book.xlsx
  File "/home/travis/build/kennethreitz/tablib/tablib/formats/_xlsx.py", line 54, in export_book
    wb.worksheets = []
AttributeError: can't set attribute

In recent openpyxl, wb.worksheets is a property (iterating over wb._sheets). To clear sheets, you can either access the "private" _sheets attribute, or delete existing sheets in a loop with public API: for sheet in wb.worksheets: wb.remove_sheet(sheet).

@claudep
Copy link
Contributor

claudep commented Feb 10, 2016

I tried to concentrate on openpyxl only (and specific test failures) in #217. If accepted, we could then do the same for other packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants