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

Small bugfix in xfile opening #60

Closed
wants to merge 2 commits into from
Closed

Small bugfix in xfile opening #60

wants to merge 2 commits into from

Conversation

t8y8
Copy link
Contributor

@t8y8 t8y8 commented Jul 23, 2016

Previously it was opening any valid xml file, despite supposedly checking for the right root tag.

Now we actually check for the root tag to see if it's in the whitelist.

@graysonarts
Copy link
Contributor

Would it be better to pass in the expected root element? It doesn't make sense that if you call Workbook("foo.tds") we wouldn't give a meaningful error. It would fail, but not with a meaningful error message.

@t8y8
Copy link
Contributor Author

t8y8 commented Jul 23, 2016

The only way to know would be to force the user to explicitly say a Workbook should be a 'workbook', which feels redundant, or go back to checking the file extension -- which I can add, and it would actually let us optimize not opening every single file in the zip, and only open *.tds *.twb files

(Not final approach, just want feedback)
(Also trying the github in web editor, so this was added blind)
@t8y8
Copy link
Contributor Author

t8y8 commented Jul 23, 2016

I think I've gotta scrap this approach and take a step back.

With the commit in its current state you can't rename a twbx to a tdsx and have it work, but you can load Workbook(TDSX_FILE) because all the xfile stuff doesn't have context for what root tag to look for. I can plumb that through, not sure how it feels yet.

@t8y8
Copy link
Contributor Author

t8y8 commented Jul 23, 2016

@RussTheAerialist I think you should move open_xml into xfile and then we can ignore this bug for 0.2.

Then we can chat about a cleaner approach to making sure Workbook is actually getting a workbook file

@graysonarts
Copy link
Contributor

graysonarts commented Jul 25, 2016

I'm okay with that. My thought was that when you invoked it from Workbook or Datasource, you would just put in the root tag you expected. That way xfile remains generic and uncoupled to the concept of Workbook/Datasource, and workbook/datasource can specify the root tag to check (and only check if specified), same for file extension, I suppose.

@t8y8
Copy link
Contributor Author

t8y8 commented Jul 25, 2016

Yeah, I like the idea of xml_open passing that argument, once you merge it I'll rebase and give it a go (I've got a long flight anyway)

@t8y8
Copy link
Contributor Author

t8y8 commented Jul 26, 2016

Replaced by #62

@t8y8 t8y8 closed this Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants