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.

Sign up for GitHub

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

Jump to bottom

CLI import_file command #725

Closed
wants to merge 8 commits into from
Closed

CLI import_file command #725

wants to merge 8 commits into from

Conversation

int-ua
Copy link
Contributor

@int-ua int-ua commented Jan 11, 2018

@coveralls
Copy link

coveralls commented Jan 11, 2018 •
edited

Coverage Status

Coverage decreased (-0.6%) to 90.63% when pulling ffe09ab on int-ua:cli-import into d0157e2 on django-import-export:master.

@coveralls
Copy link

coveralls commented Jan 11, 2018 •
edited

Coverage Status

Coverage decreased (-0.7%) to 90.582% when pulling 7825523 on int-ua:cli-import into d0157e2 on django-import-export:master.

@coveralls
Copy link

coveralls commented Jan 11, 2018 •
edited

Coverage Status

Coverage increased (+0.4%) to 91.67% when pulling ba76536 on int-ua:cli-import into d0157e2 on django-import-export:master.

@bmihelac
Copy link
Member

I did not try to run command but code looks good and it also have tests.
This would be merged after Django <1.8 support is removed.

Would it be possible to mention command in documentation and add help text for command?

@int-ua
Copy link
Contributor Author

int-ua commented Jan 11, 2018

Yes, I'll try adding docs tomorrow.

@manelclos
Copy link
Contributor

I think more tests should be added, there should be no reason to go down on coverage: Coverage decreased (-0.6%) to 90.695%

@int-ua
Copy link
Contributor Author

int-ua commented Jan 19, 2018

Please check the rows that are shown as not tested, it's some unrelated code. Or can you point what exactly is not tested in the added code?

@int-ua
Copy link
Contributor Author

int-ua commented Jan 19, 2018

Docs pending, sorry

@int-ua
Copy link
Contributor Author

int-ua commented Jan 19, 2018

Found the untested lines, I was expecting them in the "changed" tab. Will add more tests.

@int-ua
Copy link
Contributor Author

int-ua commented Feb 8, 2018

Added basic documentation and coverage should be better. Anything else?

@bmihelac bmihelac self-requested a review March 1, 2018 08:11
bmihelac
bmihelac reviewed Mar 1, 2018
View reviewed changes
import_export/management/commands/import_file.py


FORMATS = {
None: base_formats.CSV,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does None here means that CSV is default format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have time to try making it a configurable variable, should I?

@bmihelac
Copy link
Member

bmihelac commented Mar 1, 2018

Thanks @int-ua, this look good and I just have use case where I can test this thoroughly.

@ShaheedHaque
Copy link

Any chance this can be merged/released? I could really use this!

@int-ua int-ua mentioned this pull request Mar 23, 2018
Closed
@bmihelac
Copy link
Member

bmihelac commented Oct 2, 2018

@int-ua, it would be great to include this in new version. Can you resolve conflicts with master branch?

@int-ua
Copy link
Contributor Author

int-ua commented Oct 3, 2018

Yes, of course. Just not right now, I have some deadlines, unfortunately. How much time do I have until the new release?

@bmihelac
Copy link
Member

bmihelac commented Oct 3, 2018

Off course, @int-ua. When you have time :)

@ferndot ferndot mentioned this pull request Dec 28, 2018
Closed
@yozlet yozlet mentioned this pull request Jan 17, 2019
Closed
@yozlet
Copy link

yozlet commented Jan 17, 2019

I've continued this PR in #892. Many thanks, @int-ua!

@andrewgy8
Copy link
Member

@int-ua Shall we close this in favor of @yozlet 's PR #892 ?

@andrewgy8
Copy link
Member

Closing in favor of #892 which has updates and is rebased

@andrewgy8 andrewgy8 closed this Mar 23, 2019
@int-ua
Copy link
Contributor Author

int-ua commented Jul 2, 2019

Sorry for the delay, you did everything correctly, thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmihelac bmihelac bmihelac left review comments

Assignees
No one assigned
Labels
enhancement needs update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@int-ua @coveralls @bmihelac @manelclos @ShaheedHaque @yozlet @andrewgy8