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

Improve exceptions #59

Merged
merged 8 commits into from
Jun 10, 2024
Merged

Improve exceptions #59

merged 8 commits into from
Jun 10, 2024

Conversation

ColeDCrawford
Copy link
Contributor

Any other ideas of what we would want to improve here @aweakley ?

Unspecified dates previously could not handle qualification. Unspecified dates also couldn't handle dates with 3 unspecified digits ("1XXX"). This commit adds both those features and tests for those use cases.
Requires quite a few overrides of lower_ and upper_ range methods to properly handle dates due to padding working in the opposite direction for negative dates, esp when combined with month/day padding.
If not in debug mode, use a simpler EDTFParseException rather than returning the full pyparsing error
try:
if not str:
if not input_string:
raise ParseException("You must supply some input text")
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe this should be moved up out of the try: block, because otherwise you don't see the "You must supply..." message.

@aweakley
Copy link
Member

I think maybe we should catch the error and process in the same way here: https://github.com/ixc/python-edtf/blob/v5/edtf/fields.py#L135-L138

What do you think about this approach?

diff --git a/edtf/fields.py b/edtf/fields.py
index f717592..2f25c94 100644
--- a/edtf/fields.py
+++ b/edtf/fields.py
@@ -4,10 +4,12 @@ from django.core.exceptions import FieldDoesNotExist
 from django.db import models
 from django.db.models import signals
 from django.db.models.query_utils import DeferredAttribute
+from pyparsing import ParseException
 
 from edtf import EDTFObject, parse_edtf
 from edtf.convert import struct_time_to_date, struct_time_to_jd
 from edtf.natlang import text_to_edtf
+from edtf.parser.edtf_exceptions import EDTFParseException
 
 DATE_ATTRS = (
     "lower_strict",
@@ -132,10 +134,12 @@ class EDTFField(models.CharField):
         if direct_input and (
             existing_value is None or str(existing_value) != direct_input
         ):
-            edtf = parse_edtf(
-                direct_input, fail_silently=True
-            )  # ParseException if invalid; should this be raised?
-            # TODO pyparsing.ParseExceptions are very noisy and dumps the whole grammar (see https://github.com/ixc/python-edtf/issues/46)
+            try:
+                edtf = parse_edtf(
+                    direct_input, fail_silently=True
+                )  # ParseException if invalid; should this be raised?
+            except ParseException as err:
+                raise EDTFParseException(direct_input, err) from None
 
             # set the natural_text (display) field to the direct_input if it is not provided
             if natural_text == "":
diff --git a/edtf/parser/edtf_exceptions.py b/edtf/parser/edtf_exceptions.py
index 9530602..2a2dc18 100644
--- a/edtf/parser/edtf_exceptions.py
+++ b/edtf/parser/edtf_exceptions.py
@@ -2,4 +2,22 @@ from pyparsing import ParseException
 
 
 class EDTFParseException(ParseException):
-    pass
+    """Raised when an input cannot be parsed as an EDTF string.
+
+    Attributes:
+        input_string - the input string that could not be parsed
+        err -- the original ParseException that caused this one
+    """
+
+    def __init__(self, input_string, err):
+        self.input_string = input_string
+        self.err = err
+        super().__init__(err)
+
+    def __str__(self):
+        if not self.input_string or not isinstance(self.err, ParseException):
+            return super().__str__()
+        near_text = ""
+        near_text = self.input_string[max(self.err.loc - 10, 0) : self.err.loc + 10]
+        full_msg = f"Error at position {self.err.loc}: Invalid input or format near '{near_text}'. Please provide a valid EDTF string."
+        return full_msg
diff --git a/edtf/parser/grammar.py b/edtf/parser/grammar.py
index 773f806..701d398 100644
--- a/edtf/parser/grammar.py
+++ b/edtf/parser/grammar.py
@@ -357,8 +357,4 @@ def parse_edtf(input_string, parseAll=True, fail_silently=False, debug=None):
             return None
         if debug:
             raise
-        near_text = ""
-        if input_string:
-            near_text = input_string[max(err.loc - 10, 0) : err.loc + 10]
-        full_msg = f"Error at position {err.loc}: Invalid input or format near '{near_text}'. Please provide a valid EDTF string."
-        raise EDTFParseException(full_msg) from None
+        raise EDTFParseException(input_string, err) from None

ColeDCrawford and others added 2 commits June 3, 2024 15:58
Includes handling for empty or null input strings and null errs passed to the constructor

Co-Authored-By: aweakley <224316+aweakley@users.noreply.github.com>
@ColeDCrawford
Copy link
Contributor Author

Thanks for the feedback @aweakley - let me know if there's anything else needed here.

Make the string representation of TestEvent simpler
@ColeDCrawford
Copy link
Contributor Author

I think these are failing due a permissions issue again - the tests themselves seem to pass.

@aweakley
Copy link
Member

aweakley commented Jun 5, 2024

I'm seeing an error with the coverage report in my latest run here: https://github.com/ixc/python-edtf/actions/runs/9358234831/job/25820224297#step:11:1

Run MishaKav/pytest-coverage-comment@main
File read successfully "/home/runner/work/python-edtf/python-edtf/./coverage_combined.xml"
Generating coverage report
File read successfully "/home/runner/work/python-edtf/python-edtf/./combined_junit_pytest.xml"
File read successfully "/home/runner/work/python-edtf/python-edtf/./combined_junit_pytest.xml"
errors: 0
failures: 0
skipped: 0
tests: 258
time: 2.649
File read successfully "/home/runner/work/python-edtf/python-edtf/./combined_junit_pytest.xml"
Warning: Your comment is too long (maximum is 65536 characters), coverage report will not be added.
Warning: Try add: "--cov-report=term-missing:skip-covered", or add "hide-report: true", or add "report-only-changed-files: true", or switch to "multiple-files" mode
File read successfully "/home/runner/work/python-edtf/python-edtf/./combined_junit_pytest.xml"
./coverage_combined.xml
No previous comment found, creating a new one
Error: HttpError: Resource not accessible by integration
Error: Resource not accessible by integration

@ColeDCrawford
Copy link
Contributor Author

I think this is the same permissions issue as in PRs like this:

I think this is because the PRs are from a fork: MishaKav/pytest-coverage-comment#68

If you look at the workflow on my fork, you can see all the tests work and everything runs up until publishing the benchmarks, which fails because I don't have GHPages set up for the fork: https://github.com/artshumrc/python-edtf/actions/runs/9358234517/job/25759630204

@aweakley
Copy link
Member

Thank you, this looks great.

@aweakley aweakley merged commit d779dea into ixc:v5 Jun 10, 2024
0 of 5 checks passed
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.

2 participants