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

WIP Error and documentation overhaul #289

Merged
merged 6 commits into from
Mar 5, 2019
Merged

WIP Error and documentation overhaul #289

merged 6 commits into from
Mar 5, 2019

Conversation

mattcoley
Copy link
Collaborator

@mattcoley mattcoley commented Feb 27, 2019

This is a work in progress attempt to make error messaging consistent and improve documentation. On the documentation side I have split the input into a filter/expression test from the arguments for that filter/test. When the input is invalid (for example passing a string into a numeric function), we throw a InvalidInputException. When an argument is invalid we throw an InvalidArgumentException. If the number of arguments do not match the required number of arguments for the function, we throw an TemplateSyntaxException.

For invalid inputs and arguments, we specify one of the InvalidReason enum values and allow the exceptions to automatically handle building a consistent error message. For example, getting the absolute value of a string 'test'|abs would result in throw new InvalidInputException(interpreter, this, InvalidReason.NUMBER_FORMAT, object.toString());. The exception will automatically craft a nice error message:

Invalid input in 'abs': input with value 'test' must be a number.

For an invalid argument like (10|add('test')), we also pass in the argument number: throw new InvalidArgumentException(interpreter, this, InvalidReason.NUMBER_FORMAT, 0, arg[0]); which will automatically craft a message:

Invalid argument in 'add': 1st argument with value 'test' must be a number.

@bgoodies
Copy link
Contributor

🆒 👍

after this PR, can we deprecate constructors for some of the other exceptions and template error that do not use line number and start position?

}

private static String formatArgumentNumber(int argumentNumber) {
switch (argumentNumber){
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not very i18nable, but in any case, you can do this for all numbers by just looking at the last digit of the int and using "st" if 1, "nd" if 2, "rd" if 3 and "th" for all other values.

@@ -11,6 +11,8 @@

String value() default "";

JinjavaParam input();
Copy link
Contributor

Choose a reason for hiding this comment

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

while you're in here, can you add a required attribute to JinjavaParam? @williamspiro wanted one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Labeled all filter/exp tests with this new boolean value.

@codecov-io
Copy link

Codecov Report

Merging #289 into master will decrease coverage by 0.79%.
The diff coverage is 33.94%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #289     +/-   ##
===========================================
- Coverage     72.28%   71.49%   -0.8%     
- Complexity     1497     1507     +10     
===========================================
  Files           234      237      +3     
  Lines          4648     4760    +112     
  Branches        739      750     +11     
===========================================
+ Hits           3360     3403     +43     
- Misses         1033     1093     +60     
- Partials        255      264      +9
Impacted Files Coverage Δ Complexity Δ
...m/hubspot/jinjava/lib/exptest/IsNumberExpTest.java 66.66% <ø> (ø) 2 <0> (ø) ⬇️
...va/com/hubspot/jinjava/lib/filter/BatchFilter.java 79.16% <ø> (ø) 8 <0> (ø) ⬇️
...com/hubspot/jinjava/lib/filter/TruncateFilter.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...hubspot/jinjava/lib/exptest/IsSequenceExpTest.java 66.66% <ø> (ø) 2 <0> (ø) ⬇️
...om/hubspot/jinjava/lib/filter/UrlEncodeFilter.java 80.95% <ø> (ø) 8 <0> (ø) ⬇️
...bspot/jinjava/lib/exptest/IsContainingExpTest.java 100% <ø> (ø) 7 <0> (ø) ⬇️
...a/com/hubspot/jinjava/lib/filter/EscapeFilter.java 100% <ø> (ø) 5 <0> (ø) ⬇️
...va/com/hubspot/jinjava/lib/filter/LowerFilter.java 66.66% <ø> (ø) 3 <0> (ø) ⬇️
.../hubspot/jinjava/lib/exptest/IsMappingExpTest.java 66.66% <ø> (ø) 2 <0> (ø) ⬇️
...ava/com/hubspot/jinjava/lib/filter/TrimFilter.java 100% <ø> (ø) 3 <0> (ø) ⬇️
... and 101 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 815fdfd...a087022. Read the comment docs.

@mattcoley mattcoley merged commit 2aa0755 into master Mar 5, 2019
@mattcoley mattcoley deleted the error-overhaul branch March 5, 2019 15:57
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.

4 participants