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

Added additional information to exceptions #134

Merged

Conversation

roll
Copy link
Contributor

@roll roll commented Apr 23, 2020

@coveralls
Copy link

coveralls commented Apr 23, 2020

Pull Request Test Coverage Report for Build 462

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 85.423%

Totals Coverage Status
Change from base Build 458: 0.06%
Covered Lines: 1758
Relevant Lines: 2058

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 453

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 85.119%

Totals Coverage Status
Change from base Build 451: 0.07%
Covered Lines: 1716
Relevant Lines: 2016

💛 - Coveralls

@roll
Copy link
Contributor Author

roll commented Apr 23, 2020

Hi @akariv @cschloer,

I played a little bit with additional information on exceptions. It partially works but I don't fully understand how flows are composed so this implementation is really limited.

Can be a starting point for a better implementation

@cschloer
Copy link
Contributor

This looks good, and pretty much exactly what I was getting at, but I see that it's only limited to processors that inherit the DataStreamProcessor class. Is there any way to extrapolate this out to the flow.py file? So it works by default for all processors, even custom ones that don't inherit any datahq code? I also am not familiar enough with how flows work to get a sense of whether that is possible.

@roll
Copy link
Contributor Author

roll commented May 11, 2020

hi @akariv WDYT?

@roll
Copy link
Contributor Author

roll commented May 19, 2020

As decided on the prev meeting - it's parked for now to get @akariv input first

@akariv
Copy link
Member

akariv commented May 20, 2020

@roll - sorry for missing this, it's really great!

Regarding the implementation -

  • The added information to the exceptions is neat, but most users won't notice them.
    I wonder if we could create a 'DataflowsException' which will have these attributes and will be chained to the original exception (using the raise ... from e syntax)
  • Flow composition can be done using chaining processors or Flows.
    The stored position is the processor in the flow, but it might get confusing when combining a number of flows together.
    Perhaps we could have the position to be a combination of Flow + position in flow, but for now I think the implementation is fine.
  • row_processor, rows_processor, datapackage_processor, iterable_loader - all return an instance of DataStreamProcessor so all should get the position parameter as well

@roll roll force-pushed the exception-information branch from cfb6765 to 2357587 Compare May 25, 2020 13:14
@roll roll changed the title [WIP] Added additional information to exceptions Added additional information to exceptions May 25, 2020
@roll
Copy link
Contributor Author

roll commented May 25, 2020

Hi @akariv,

I've added the exception objects you mentioned. Also, updated the implementation to make it work with different combinations of processors. TBH not sure how it works but it seems to be working

@akariv akariv changed the base branch from master to feature/exception-information May 26, 2020 14:12
@akariv akariv merged commit f3dd15f into datahq:feature/exception-information May 26, 2020
@roll roll deleted the exception-information branch May 26, 2020 14:17
akariv added a commit that referenced this pull request May 26, 2020
* Added additional information to exceptions (#134)

* Added additional information to exceptions

* Updated implementation

Co-authored-by: Adam Kariv <[email protected]>

* Complete exceptions work on other dsp kinds

* Fix tests

Co-authored-by: roll <[email protected]>
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.

Add processor (?and row?) information to exception object
4 participants