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

deals with .xls older file extension #4200

Merged
merged 4 commits into from Aug 11, 2017

Conversation

Projects
None yet
6 participants
@FredericBGA
Copy link
Contributor

commented Jun 16, 2017

I need to deal with old .xls files for some users.
I'm not sure if all lines of code are really mandatory.

@galaxybot galaxybot added the triage label Jun 16, 2017

@galaxybot galaxybot added this to the 17.09 milestone Jun 16, 2017

@nsoranzo
Copy link
Member

left a comment

You will also have to change the line:

<datatype extension="xls" type="galaxy.datatypes.tabular:Tabular"/>

in config/datatypes_conf.xml.sample
to:

<datatype extension="xls" type="galaxy.datatypes.binary:Xls" display_in_upload="true" />

def sniff( self, filename ):
ret = os.popen("file -i " + filename)
mimeType = ret.read().rstrip()

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jun 16, 2017

Member

Please use the subprocess module instead, e.g. mimeType = subprocess.check_output("file -i %s" % filename).rstrip()

This comment has been minimized.

Copy link
@FredericBGA

FredericBGA Jun 16, 2017

Author Contributor

ok, I'll change this

def sniff( self, filename ):
ret = os.popen("file -i " + filename)
mimeType = ret.read().rstrip()
if (mimeType.find("application/vnd.ms-excel") != -1):

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jun 16, 2017

Member

The external parentheses are not needed.

This comment has been minimized.

Copy link
@FredericBGA

FredericBGA Jun 16, 2017

Author Contributor

I'll change this too.
I'm not yet really good with Python...

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

@galaxybot test this

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

Ping @blankenberg which initially added the xls fake datatype in commit c8a71a4.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 5, 2017

I have opened a PR with a variety of changes against the source branch here FredericBGA#1. @FredericBGA please have a look at the details and let me know if everything there looks okay. I'm not unwilling to drop support for legacy XLS datatype within Galaxy - I can't recall what it is used for - but the approach I PR'd would prevent us from needing to so if that is good enough we don't even need to worry about it further.

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jul 5, 2017

And let's ping @blankenberg again!

@blankenberg

This comment has been minimized.

Copy link
Member

commented Jul 10, 2017

IIRC the tabular xls type was added as part of the RGenetics work that I think has since been deprecated, although there may still be instances that are using these datatypes, and I'd predict that some of the tools will make a comeback at some point.

I'm always in favor of maintaining backwards compatibility, so creating a new datatype seems the safer route here. But I don't have a specific argument for keeping the tabular based xls filetype around.

The PR by @jmchilton is required to have the file command work correctly on a mac. I'm not a huge fan of using file in general for these tests, I'd rather check that the magic bytes are a ms doc, then do the required deeper investigation into the file to determine the subtype.

Regardless, the call to subprocess should be using an argument list, and not string formatting, since it will fail when there are e.g. spaces in the path.

@jmchilton jmchilton referenced this pull request Aug 10, 2017

Merged

Add Excel97 Datatype #4410

@bgruening bgruening merged commit 93dadd6 into galaxyproject:dev Aug 11, 2017

4 of 5 checks passed

api test Build finished. 279 tests run, 0 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.