Skip to content

Add functionality so that if one entry breaks during a batch, it will…#9

Merged
jamesuejio merged 1 commit intomasterfrom
update-batch-entry-logic
Aug 7, 2020
Merged

Add functionality so that if one entry breaks during a batch, it will…#9
jamesuejio merged 1 commit intomasterfrom
update-batch-entry-logic

Conversation

@jamesuejio
Copy link

… not crash the entire batch

@jamesuejio jamesuejio requested a review from a team August 6, 2020 01:57
@jamesuejio jamesuejio added the cc-tech debt Paying out technical debt. Examples: refactoring features for better extensibility. label Aug 6, 2020
ach/builder.py Outdated
entry_counter += 1
entries.append((entry, record.get('addenda', [])))
entry_counter += 1
except:

Choose a reason for hiding this comment

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

Can we return what type of failure it was?

Swallowing the error on a bunch of lines will help but, I believe we can do a better job by informing the user what the errors are or where they happened.

Copy link
Author

Choose a reason for hiding this comment

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

Sure how do you think we should return the error? Should I add the error message in the entry? Or have failed_entries a tuple of entry dict and error message?

Choose a reason for hiding this comment

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

I like the tuple idea.

Choose a reason for hiding this comment

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

Tuple is ok for now but we could return an either type in the future.

Choose a reason for hiding this comment

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

Hmm, Felippe, looking closer at your comment you're totally right. Can we log it with some kind of unique identifier to find that detail if not obvious from the data itself returned to the caller? This is a library so not sure what the convention is in python for logging in libraries, thoughts?

@jamesuejio jamesuejio force-pushed the update-batch-entry-logic branch 3 times, most recently from 13ff82d to 8b240c9 Compare August 7, 2020 18:52
@jamesuejio jamesuejio force-pushed the update-batch-entry-logic branch from 8b240c9 to c116468 Compare August 7, 2020 18:57
@natalya-carta
Copy link

Changes look awesome, I feel like there should be an obvious test we can add?

Copy link

@natalya-carta natalya-carta left a comment

Choose a reason for hiding this comment

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

LGTM

@jamesuejio
Copy link
Author

Oh there are tests in this repo! Let me add a quick one

@jamesuejio
Copy link
Author

Actually can't figure out how to run them and the last commits to tests were 6 years so not sure it's worth trying to figure it out.

@jamesuejio jamesuejio merged commit 9585d30 into master Aug 7, 2020
@carta carta deleted a comment from natalya-carta Aug 10, 2020
@carta carta deleted a comment from natalya-carta Aug 10, 2020
@jamesuejio jamesuejio deleted the update-batch-entry-logic branch August 14, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cc-tech debt Paying out technical debt. Examples: refactoring features for better extensibility.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants