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

✨ Add error messages for constraint failures #1036

Merged
merged 6 commits into from Sep 28, 2023

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Sep 25, 2023

closes #1011

@danwos I felt the name help was not very descriptive of its purpose, and having the config key as error_message and the needs field constraints_error was more understandable. What do you think?

Also perhaps:

  • Add documentation
  • A try/catch for errors in the jinja templating
  • Including the message in the sphinx warning

I'd note that, in principle, I feel it would be more "correct" to lay out the configuration like this:

needs_constraints = {
   "name": {
       "checks": {
            "0": "'critical' in tags",
		},
		"severity": "CRITICAL",
        "error_message": "need {{id}} does not fulfill CRITICAL constraint, because tags are {{tags}}",
   }
}

i.e. having the checks under a namespace, rather than having "special" keys

@chrisjsewell chrisjsewell marked this pull request as ready for review September 25, 2023 17:03
@chrisjsewell chrisjsewell changed the title ✨ Add error messages for constrain failures ✨ Add error messages for constraint failures Sep 25, 2023
Copy link
Member

@danwos danwos left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Have only one suggesting regarding performance optimization.

@@ -45,6 +46,11 @@ def process_constraints(app: Sphinx, need: NeedsInfoType) -> None:
need["constraints_results"].setdefault(constraint, {})[name] = False
need["constraints_passed"] = False

if "error_message" in executable_constraints:
need["constraints_error"] = jinja2.Template(str(executable_constraints["error_message"])).render(
Copy link
Member

Choose a reason for hiding this comment

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

Initiating a jinja template takes some time, can we do it only once and execute only the rendering per need?

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Sep 26, 2023

Initiating a jinja template takes some time, can we do it only once and execute only the rendering per need?

Thanks @danwos, this pushed me to do something I was already planning to do in #1018: process all constraints in a single pass, rather than per document.

Do you know any reason why it was per document in the first place?
In fact, from the snapshots, this seems to fix some issues, where some constraints were not getting processed 😄

@danwos
Copy link
Member

danwos commented Sep 27, 2023

No, can't say (anymore) why it was implemented like this.
@mawieland As the initial implementer of this feature, do you remember or see any constraints?

@chrisjsewell chrisjsewell merged commit d06f3a6 into master Sep 28, 2023
12 checks passed
@chrisjsewell chrisjsewell deleted the 1011/constraint-help branch September 28, 2023 10:22
iSOLveIT pushed a commit that referenced this pull request Oct 24, 2023
This commit adds the ability to set an optional `error_message` key for each constraint in the `needs_constraint` configuration.
This can contain Jinja placeholders, for needs data fields, and will be added to the need data, under the `constraints_error` field, if the constraint fails.
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.

Feature: Constraint help for error cases
2 participants