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

JSON export #28

Merged
merged 20 commits into from Jun 7, 2018

Conversation

Projects
None yet
2 participants
@duskybomb
Collaborator

duskybomb commented May 29, 2018

Features:

  • extracts fields which have attribute _required = true
  • for both factur-x and zugfred
  • extracts xml from external xml files
  • export embedded xml from PDFs.

for issue #4

@duskybomb

This comment has been minimized.

Show comment
Hide comment
@duskybomb

duskybomb May 29, 2018

Collaborator

All tasks done now you can review this, whenever free @m3nu.

Collaborator

duskybomb commented May 29, 2018

All tasks done now you can review this, whenever free @m3nu.

@duskybomb duskybomb referenced this pull request Jun 2, 2018

Closed

Implement validation #5

Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
@m3nu

This comment has been minimized.

Show comment
Hide comment
@m3nu

m3nu Jun 2, 2018

Collaborator

Can't merge this now. Needs major changes. See comments in files.

Collaborator

m3nu commented Jun 2, 2018

Can't merge this now. Needs major changes. See comments in files.

@duskybomb

This comment has been minimized.

Show comment
Hide comment
@duskybomb

duskybomb Jun 2, 2018

Collaborator

Almost all the suggestions have been taken care of. The only thing is how not to hard-code namespaces and a way to remove nested IF (the problem is I am looping after every IF condition)

Collaborator

duskybomb commented Jun 2, 2018

Almost all the suggestions have been taken care of. The only thing is how not to hard-code namespaces and a way to remove nested IF (the problem is I am looping after every IF condition)

@m3nu

This comment has been minimized.

Show comment
Hide comment
@m3nu

m3nu Jun 2, 2018

Collaborator

There is still flavor-specific logic.

Collaborator

m3nu commented Jun 2, 2018

There is still flavor-specific logic.

@duskybomb

This comment has been minimized.

Show comment
Hide comment
@duskybomb

duskybomb Jun 2, 2018

Collaborator

Added a new commit.

Collaborator

duskybomb commented Jun 2, 2018

Added a new commit.

Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
@m3nu

This comment has been minimized.

Show comment
Hide comment
@m3nu

m3nu Jun 3, 2018

Collaborator

None of my input was addressed. All your loops are a mess. If you can't do it any better, just leave it open for me to rewrite it. Just can't be merged like this ever.

Collaborator

m3nu commented Jun 3, 2018

None of my input was addressed. All your loops are a mess. If you can't do it any better, just leave it open for me to rewrite it. Just can't be merged like this ever.

@duskybomb

This comment has been minimized.

Show comment
Hide comment
@duskybomb

duskybomb Jun 3, 2018

Collaborator

I understand, I have now addressed all the flavor specific issues. I don't understand what you mean by "loops are a mess". Can you explain?

Collaborator

duskybomb commented Jun 3, 2018

I understand, I have now addressed all the flavor specific issues. I don't understand what you mean by "loops are a mess". Can you explain?

duskybomb added some commits Jun 3, 2018

Changes after review:
 * Check condition if list or dict is required
 * Remove flavor based code
 * Fewer nested IFs
 * Small improvements
Show outdated Hide outdated facturx/facturx.py
@m3nu

This comment has been minimized.

Show comment
Hide comment
@m3nu

m3nu Jun 6, 2018

Collaborator

In this PR you are duplicating functionality that already exists in other parts of the class. Like parsing an XML file.

You also made very complicated methods that require more input instead of just using the current class instance. If you do it like this, there is no point in using an OO design.

So most code except for __make_dict() can be removed. We only need __make_dict() + some code to save the json file.

Collaborator

m3nu commented Jun 6, 2018

In this PR you are duplicating functionality that already exists in other parts of the class. Like parsing an XML file.

You also made very complicated methods that require more input instead of just using the current class instance. If you do it like this, there is no point in using an OO design.

So most code except for __make_dict() can be removed. We only need __make_dict() + some code to save the json file.

Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py
Show outdated Hide outdated facturx/facturx.py

duskybomb added some commits Jun 7, 2018

@m3nu m3nu merged commit c12878e into master Jun 7, 2018

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