Make error message from parsing more explicit #437

Ananya2003Gupta · 2023-06-29T18:14:19Z

BEGINRELEASENOTES

Modified parse function definition to incorporate missing description in member definition error message. Fixes Misleading Error: Invalid Member Definition #436

ENDRELEASENOTES
Solves Issue #436 : Misleading Error: Invalid Member Definition

… in member definition error message.

tmadlener

Thanks for taking care of this. I triggered continuous integration (CI), and I have some minor suggestions below.

tmadlener · 2023-06-29T18:19:30Z

python/podio/podio_config_reader.py

+        self._parse_with_regexps(string, no_desc_matchers_cbs)
+        raise DefinitionError(f"'{string}' is not a valid member definition. Description (Comment after member definition) is missing. Correct Syntax: <type> <name> // <comment>")  #require_description is set to True but member definition is missing description


Suggested change

self._parse_with_regexps(string, no_desc_matchers_cbs)

raise DefinitionError(f"'{string}' is not a valid member definition. Description (Comment after member definition) is missing. Correct Syntax: <type> <name> // <comment>") #require_description is set to True but member definition is missing description

# check whether we could parse this if we don't require a description and provide more details in the error if we can

self._parse_with_regexps(string, no_desc_matchers_cbs)

raise DefinitionError(f"'{string}' is not a valid member definition. Description comment is missing. Correct Syntax: <type> <name> // <comment>")

I would move the comment to the top of these lines, because people are more likely to find it there.

python/podio/podio_config_reader.py

Ananya2003Gupta · 2023-06-29T18:25:06Z

Thanks for taking care of this. I triggered continuous integration (CI), and I have some minor suggestions below.

I didn't do anything. The additions were suggested by you only : )

tmadlener · 2023-06-29T18:35:03Z

It looks like our format and lint checks are not entirely happy yet:

https://github.com/AIDASoft/podio/actions/runs/5415352399/jobs/9843528142?pr=437#step:4:499

The rest looks fine. There is one workflow that is known to fail and which you can ignore in this case (key4hep).

Ananya2003Gupta · 2023-06-29T18:39:43Z

It looks like our format and lint checks are not entirely happy yet:

https://github.com/AIDASoft/podio/actions/runs/5415352399/jobs/9843528142?pr=437#step:4:499

The rest looks fine. There is one workflow that is known to fail and which you can ignore in this case (key4hep).

Following errors are generated.

python/podio/podio_config_reader.py:89:13: E126 continuation line over-indented for hanging indent
  python/podio/podio_config_reader.py:91:9: E121 continuation line under-indented for hanging indent
  python/podio/podio_config_reader.py:94:21: E126 continuation line over-indented for hanging indent
  python/podio/podio_config_reader.py:96:7: E121 continuation line under-indented for hanging indent
  python/podio/podio_config_reader.py:103:121: E501 line too long (261 > 120 characters)
  python/podio/podio_config_reader.py:103:182: E262 inline comment should start with '# '

tmadlener

You will have to fix the format issues to make that check happy. I have marked most of the occurrences below. If you have flake8 and pre-commit installed locally you can run pre-commit locally. You can also just do pre-commit run -a flake8 to just run the flake8 checks (which is much quicker than the full suite).

tmadlener · 2023-06-29T18:58:00Z

python/podio/podio_config_reader.py

+            (self.full_array_re, self._full_array_conv),
+            (self.member_re, self._full_member_conv)


Here flake8 complains about too much indentation. So you will have to unindent these (to the same level they were before)

tmadlener · 2023-06-29T18:58:43Z

python/podio/podio_config_reader.py

+                    (self.bare_array_re, self._bare_array_conv),
+                    (self.bare_member_re, self._bare_member_conv)
+      ]


Here similar indentation issues are present.

tmadlener

Thanks for fixing the formatting. I just realized that there are also some pylint warnings, which should be fixed by the comments below.

tmadlener · 2023-06-29T20:14:28Z

python/podio/podio_config_reader.py

+        """check whether we could parse this if we don't require a description and
+        provide more details in the error if we can"""


Suggested change

"""check whether we could parse this if we don't require a description and

provide more details in the error if we can"""

# check whether we could parse this if we don't require a description and

# provide more details in the error if we can

Otherwise pylint complains about a string statement without effect: log

python/podio/podio_config_reader.py

tmadlener · 2023-06-29T20:19:18Z

python/podio/podio_config_reader.py

+        raise DefinitionError(f"""'{string}' is not a valid member definition. Description comment is missing.
+Correct Syntax: <type> <name> // <comment>""")


Suggested change

raise DefinitionError(f"""'{string}' is not a valid member definition. Description comment is missing.

Correct Syntax: <type> <name> // <comment>""")

raise DefinitionError(f"'{string}' is not a valid member definition. Description comment is missing."

" Correct Syntax: <type> <name> // <comment>")

You can also split strings this way and they will be concatenated. I find this a bit more readable.

Ananya2003Gupta · 2023-06-30T08:17:12Z

Doing the above changes.
Really sorry. I just ran the flake8 tests and committed the changes.

tmadlener · 2023-06-30T08:55:18Z

python/podio/podio_config_reader.py

+        raise DefinitionError(f"'{string}' is not a valid member definition. Description comment is missing.\n"
+                               + "Correct Syntax: <type> <name> // <comment>")


Suggested change

raise DefinitionError(f"'{string}' is not a valid member definition. Description comment is missing.\n"

+ "Correct Syntax: <type> <name> // <comment>")

raise DefinitionError(f"'{string}' is not a valid member definition. Description comment is missing."

" Correct Syntax: <type> <name> // <comment>")

You don't need the + in this case, and I would also not put in a newline (\n) to the output. Other than that this looks good.

I put the newline character so that the error message looks good.
With newline character

Without newline character

Without newline character the error doesn't look good on screen.

Also, I put a + for concatenation of string.

OK. I like the newline. But the + is not necessary for string concatenation in this case, because it will be done automatically inside the ().

Okay, I'll keep the newline character and remove + .

tmadlener · 2023-06-30T09:26:08Z

Just waiting for the last few workflows to finish, but they should be fine. Thanks again for all the work (and the patience in addressing my comments).

If I may ask: How did you get into contact with podio?

Ananya2003Gupta · 2023-06-30T09:29:49Z

Just waiting for the last few workflows to finish, but they should be fine. Thanks again for all the work (and the patience in addressing my comments).

It was really fun working on this issue and I got to learn a lot about pre-commit through it.

If I may ask: How did you get into contact with podio?

I am selected as HSF India Trainee (IRIS HEP Fellow) and will be working on interfacing PODIO with Julia backend.

tmadlener · 2023-06-30T09:34:46Z

Ah very nice. We will be hearing more from you then :)

Ananya2003Gupta · 2023-06-30T09:35:24Z

Ah very nice. We will be hearing more from you then :)

Yes : )

Ananya2003Gupta and others added 4 commits June 29, 2023 23:06

Create cmake.yml

f618441

removed cmake.yml

5cfd975

Modified parse function definition to incorporate missing description…

00dd9cd

… in member definition error message.

added explanatory comment to parse function

1a98e9b

tmadlener reviewed Jun 29, 2023

View reviewed changes

tmadlener linked an issue Jun 29, 2023 that may be closed by this pull request

Misleading Error: Invalid Member Definition #436

Closed

tmadlener reviewed Jun 29, 2023

View reviewed changes

fixed indentation issues of code

f7e3c10

tmadlener reviewed Jun 29, 2023

View reviewed changes

fixed pylint warnings

e85ecca

tmadlener reviewed Jun 30, 2023

View reviewed changes

remove + operator

711d967

tmadlener changed the title ~~Solves Issue #436 : Misleading Error: Invalid Member Definition~~ Make error message from parsing more explicit Jun 30, 2023

tmadlener merged commit d8294d2 into AIDASoft:master Jun 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make error message from parsing more explicit #437

Make error message from parsing more explicit #437

Ananya2003Gupta commented Jun 29, 2023 •

edited by tmadlener

Loading

tmadlener left a comment

tmadlener Jun 29, 2023

Ananya2003Gupta commented Jun 29, 2023

tmadlener commented Jun 29, 2023

Ananya2003Gupta commented Jun 29, 2023

tmadlener left a comment

tmadlener Jun 29, 2023

tmadlener Jun 29, 2023

tmadlener left a comment

tmadlener Jun 29, 2023

tmadlener Jun 29, 2023

Ananya2003Gupta commented Jun 30, 2023

tmadlener Jun 30, 2023

Ananya2003Gupta Jun 30, 2023

Ananya2003Gupta Jun 30, 2023

tmadlener Jun 30, 2023

Ananya2003Gupta Jun 30, 2023

tmadlener commented Jun 30, 2023

Ananya2003Gupta commented Jun 30, 2023 •

edited

Loading

tmadlener commented Jun 30, 2023

Ananya2003Gupta commented Jun 30, 2023

		self._parse_with_regexps(string, no_desc_matchers_cbs)
		raise DefinitionError(f"'{string}' is not a valid member definition. Description (Comment after member definition) is missing. Correct Syntax: <type> <name> // <comment>") #require_description is set to True but member definition is missing description

		(self.full_array_re, self._full_array_conv),
		(self.member_re, self._full_member_conv)

		"""check whether we could parse this if we don't require a description and
		provide more details in the error if we can"""

		raise DefinitionError(f"""'{string}' is not a valid member definition. Description comment is missing.
		Correct Syntax: <type> <name> // <comment>""")

		raise DefinitionError(f"'{string}' is not a valid member definition. Description comment is missing.\n"
		+ "Correct Syntax: <type> <name> // <comment>")

Make error message from parsing more explicit #437

Make error message from parsing more explicit #437

Conversation

Ananya2003Gupta commented Jun 29, 2023 • edited by tmadlener Loading

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ananya2003Gupta commented Jun 29, 2023

tmadlener commented Jun 29, 2023

Ananya2003Gupta commented Jun 29, 2023

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ananya2003Gupta commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmadlener commented Jun 30, 2023

Ananya2003Gupta commented Jun 30, 2023 • edited Loading

tmadlener commented Jun 30, 2023

Ananya2003Gupta commented Jun 30, 2023

Ananya2003Gupta commented Jun 29, 2023 •

edited by tmadlener

Loading

Ananya2003Gupta commented Jun 30, 2023 •

edited

Loading