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

Missing Cell Handling #45

Open
dietzc opened this issue Jul 14, 2016 · 7 comments
Open

Missing Cell Handling #45

dietzc opened this issue Jul 14, 2016 · 7 comments

Comments

@dietzc
Copy link
Member

dietzc commented Jul 14, 2016

If MissingValue cells are mapped into a module, we should handle this gracefully. If null is returned by the module, we should return a MissingCell.

@Squareys Squareys modified the milestone: knip-scijava-1.0.0-beta Sep 6, 2016
@Squareys
Copy link
Contributor

Theoretically, this is handled by the Converter Framework. That depends on how they are cached. If you require a converter from MissingValue -> (Any) Object, you automatically get a converter which always returns null.

I wonder how that works for the other way around... Since you cannot do nulledObject.class and null instanceof Object == false.

  • Check Converter Framework null to MissingValue conversion.

@Squareys
Copy link
Contributor

I added a test: MissingValue handling seems to work perfectly fine in #49.

@dietzc
Copy link
Member Author

dietzc commented Sep 13, 2016

What happens for incoming MissingValues? Actually, the node should fail gracefully with a nice error message if a required=true input is null. For required=false the node shouldn't fail.

If it produces MissingValues as output, we have to decide what we do: Create missing-cells for all inputs or let the node fail?. I prefer fail with the corresponding error-message from the Command in the console log because it's more transparent to the user (node isn't green and has tons of MissingValues.). @hornm any comments?

@Squareys
Copy link
Contributor

Squareys commented Sep 14, 2016

Actually, the node should fail gracefully with a nice error message if a required=true input is null. For required=false the node shouldn't fail.

That's an important note. This basically means that MissingValues should not be allowed to even get to the Converter Framework, but handled beforehand?

If it produces MissingValues as output, we have to decide what we do: Create missing-cells for all inputs (outputs?) or let the node fail?

Why for all outputs? Some outputs may still be valid and useful? We can still put an error message into the missing cell and possibly produce a Warning.

@dietzc
Copy link
Member Author

dietzc commented Sep 14, 2016

That's an important note. This basically means that MissingValues should not be allowed to even get to the Converter Framework, but handled beforehand?

for required=true MissingValues shouldn't be allowed.

Why for all outputs? Some outputs may still be valid and useful? We can still put an error message into the missing cell and possibly produce a Warning.

Sounds good, however I'd avoid to produce warnings for each row. Rather I'd suggest that we have a warning summary after the node has finished (like K-Errors and N-Warnings occured, see log for details). And in the log we have more detailed messages on e.g. log-debug level.

@Squareys
Copy link
Contributor

I'd avoid to produce warnings for each row

How about: "K-Errors and N-Warnings occured, see missing cells for details" :P

@dietzc
Copy link
Member Author

dietzc commented Sep 22, 2016

I think we are a bit off-topic now: So for MissingCell input/output the strategy is clear I guess. The Exception handling will be discussed in #53.

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

No branches or pull requests

2 participants