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

Python importer #6

Merged
merged 2 commits into from
Nov 30, 2016
Merged

Python importer #6

merged 2 commits into from
Nov 30, 2016

Conversation

cboulay
Copy link
Collaborator

@cboulay cboulay commented Nov 22, 2016

No guarantees this works. The API may change. But please test!

@cbrnr
Copy link
Contributor

cbrnr commented Nov 30, 2016

Very nice to see some Python code here! I've ran the code through some quick test files. For a first version I think this works very well.

A few things could be changed though. For example, I suggest to use the logging module to do all the logging stuff which is now done with the print function. In addition, verbose should really be False by default as described in the docstring.

Furthermore, I don't know if the use of defaultdict is really necessary. Printing defaultdicts is not really nice because the output includes a lot of boilerplate strings. I haven't looked at the code closely enough, but maybe it is possible to use normal dicts instead of defaultdicts. Furthermore, many entries are one-element lists, which makes accessing these elements rather difficult. We should check if lists are really necessary for all fields or if there are fields that can only have one entry - in that case, we could get rid of these one-element lists.

I'd prefer the docstrings to adhere to the numpydoc format.

I'd be happy to contribute if you want. I suggest to merge this PR into master if there are no serious bugs (and so far I don't see any) so that we can deal with changes in separate PRs.

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.

None yet

3 participants