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

Fix for Issue #724 #1080

Closed
wants to merge 9 commits into from
Closed

Fix for Issue #724 #1080

wants to merge 9 commits into from

Conversation

mehak16163
Copy link

@mehak16163 mehak16163 commented May 27, 2020

For this fix of issue #724 I have

  1. Added ignore_error method for each of the parsers included in rdflib/plugins/parsers.
  2. Added a couple of test cases to verify whether the new keyword works as expected.
  3. In graph.py where the parse method is called, I have made the changes to include the keyword.
  4. Added the logging functionality when the ignore_errors = True, so that we can identify the lines which were not correctly parsed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 75.801% when pulling 4300559 on mehak16163:Issue724 into 037ea51 on RDFLib:master.

@mehak16163
Copy link
Author

Hi @nicholascar,
Can you please go through this pull request and suggest any changes to be done?

@nicholascar
Copy link
Member

Hi @mehak16163, thanks for this PR but there are some problems with it that mean we, the maintainers, have to reject it.

It doesn't make sense to ignore parsing errors in any RDF format other than n-triples as the triples are inter-connected so if you ignore a parsing error, you will not get a valid graph. in n-triples, you could ignore a single line parsing fail as each line is a subject, predicate, object, triple, but this is the only format you can do this for.

Also, you've tried to present a passing and a failing test for your changes but your passing test will always pass and doesn't notice what the state of the parsed RDF is, since you've ignored errors, and your failing test will always fail, since you are parsing invalid RDF. So your given tests don't actually tell us the code is working (the code can't work, except for n-triples, as I said).

Sorry but I have to close this PR.

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.

3 participants