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

passing array=pyexcel.Sheet to pyexcel.save_as is an un-intended use #22

Closed
chfw opened this issue Apr 4, 2016 · 7 comments
Closed

passing array=pyexcel.Sheet to pyexcel.save_as is an un-intended use #22

chfw opened this issue Apr 4, 2016 · 7 comments

Comments

@chfw
Copy link
Member

@chfw chfw commented Apr 4, 2016

Hi all,

I just realize that passing array=pyexcel.Sheet is an un-intended use. And these code changes are against the interface between pyexcel-io and pyexcel is a dictionary of two dimensional array. I am sorry that this interface is not documented publicly. The reason for such an interface is to break the tight coupling between pyexcel and pyexcel-io.

https://github.com/pyexcel/pyexcel-text/blob/master/pyexcel_text/__init__.py#L125
https://github.com/pyexcel/pyexcel-text/blob/master/pyexcel_text/__init__.py#L185

I guess this line has played a mis-leading role in it:

https://github.com/pyexcel/pyexcel-text/blob/master/tests/test_io.py#L208

With that said, pyexcel-text is an exception among the pyexcel-io plugins because it also serves as a presentation plugin to pyexcel, in other words, pyexcel-text is a tightly coupled with pyexcel.

So, I will need to find a way to in-corporate the new use cases, either making this un-intended use as intended or implementing them within the interface set out.

@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Apr 4, 2016

Hmm. If it helps, I am happy to pin my repo dependency to the current revision where my use works correctly, so you can break it in master

@chfw
Copy link
Member Author

@chfw chfw commented Apr 5, 2016

Please hang on the current commit in master. I found a way to incorporate your use cases and will update this thread in a few days

@chfw
Copy link
Member Author

@chfw chfw commented Apr 7, 2016

Please evaluate current pyexcel-text with pyexcel v0.2.1.

I change its extension mechanism. It used to extend pyexcel-io but now it extends from pyexcel.source.FileSource, where structure context is available.

@chfw
Copy link
Member Author

@chfw chfw commented Apr 7, 2016

pyexcel v0.2.1 will try to load pyexcel-text if it is installed. hence, you won't need to import it. The same will happen to pyexcel-io plugins when pyexcel-io v0.2.0 will be released.

@chfw chfw closed this in b32b220 Apr 23, 2016
@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented May 3, 2016

I've retested using the latest pyexcel-text and pyexcel, and it works for my use after the following change:

-python -c "import pyexcel, pyexcel.ext.text, pyexcel.ext.xlsx; sheet = pyexcel.sheets.NominableSheet(pyexcel.get_sheet(file_name='downloads/ERA2012JournalList.xlsx'), name_columns_by_row=0); pyexcel.save_as(array=sheet, dest_file_name='downloads/ERA2012JournalList.json')"
+python -c "import pyexcel, pyexcel.ext.xlsx; pyexcel.save_as(file_name='downloads/ERA2012JournalList.xlsx', dest_file_name='downloads/ERA2012JournalList.json', name_columns_by_row=0)"

The new json file is identical to the old one.

The old command now produces exceptions (AttributeError if I recall correctly), so the new version isnt backwards compatible, but my new command is obviously much better, and is roughly what I expected I should write when I first tried to use pyexcel. So I'm happy it now works and is readable.
Thanks.

@chfw
Copy link
Member Author

@chfw chfw commented May 11, 2016

Please be aware that this commit 4308dec will affect your downstream projects. Mainly it is because of pyexcel/pyexcel#32, pyexcel/pyexcel#35

@chfw
Copy link
Member Author

@chfw chfw commented Jun 1, 2016

pyexcel-text v0.2.2 is released. Will have an impact on your projects

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.