Skip to content

Report a compilation error for invalid placeholders [Part I]#216

Merged
yevhenii-nadtochii merged 19 commits intomasterfrom
report-illegal-placeholders
Apr 29, 2025
Merged

Report a compilation error for invalid placeholders [Part I]#216
yevhenii-nadtochii merged 19 commits intomasterfrom
report-illegal-placeholders

Conversation

@yevhenii-nadtochii
Copy link
Copy Markdown
Contributor

@yevhenii-nadtochii yevhenii-nadtochii commented Apr 25, 2025

This PR addresses #213 for boolean options that use a companion option to specify custom error messages.

The following options have received this compilation check: (distinct), (required), (set_once).

Their companion policies now extract and check the error messages on their own. Instead of a single event, the main and companion policies now emit their own events that are accumulated in the view:

  1. RequiredFieldDiscoveredRequiredFieldDiscovered and IfMissingOptionDiscovered.
  2. DistinctFieldDiscoveredDistinctFieldDiscovered and IfHasDuplicatesOptionDiscovered.
  3. SetOnceFieldDiscoveredSetOnceFieldDiscovered and IfSetAgainOptionDiscovered.

Note that we usually emit the following event: ${OptionName}{OptionTarget}Discovered. I kept this convention for the main options, but emitting something like IfMissingFieldDiscovered makes no sense because this is actually an option that affects another, its main option, not the field itself.

Having these checks in :model required the following changes:

  1. The ErrorPlaceholder enum has been moved to :model.
  2. resolveErrorMessage() function is no longer needed. Companion policies now unpack the option and extract the message in a typed manner.
  3. CompanionPolicy has been replaced in favor of the checkBothApplied() function.

As for now, I haven't extracted a common checkPlaceholders() function to check for placeholders. I'll do in the next PR when the rest of options will be given such a check.

Non-related changes

ChoiceView has been renamed to ChoiceGroupView to fix naming consistency: ${OptionName}{OptionTarget}View.

@yevhenii-nadtochii yevhenii-nadtochii changed the title Report compilation error for invalid placeholders [Part I] Report a compilation error for invalid placeholders [Part I] Apr 25, 2025
@yevhenii-nadtochii yevhenii-nadtochii self-assigned this Apr 25, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 25, 2025

Codecov Report

Attention: Patch coverage is 68.86792% with 33 lines in your changes missing coverage. Please review.

Project coverage is 26.14%. Comparing base (b266ea3) to head (fe9a0d7).
Report is 20 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #216      +/-   ##
============================================
+ Coverage     23.67%   26.14%   +2.47%     
- Complexity      240      241       +1     
============================================
  Files           137      137              
  Lines          3177     3236      +59     
  Branches        241      247       +6     
============================================
+ Hits            752      846      +94     
+ Misses         2392     2351      -41     
- Partials         33       39       +6     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yevhenii-nadtochii yevhenii-nadtochii marked this pull request as ready for review April 28, 2025 11:02
@yevhenii-nadtochii
Copy link
Copy Markdown
Contributor Author

@armiol @alexander-yevsyukov PTAL

Copy link
Copy Markdown
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@yevhenii-nadtochii LGMT in general. Please see my comment though.

*/
internal fun checkBothApplied(
companion: GeneratedExtension<*, *>,
primary: GeneratedExtension<*, *>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of having a function taking two very similar arguments (in which I would have the primary first, BTW), I'd rather have an extension function to GeneratedExtension.

The reason is that the invocations of callFunction(someparameter, someotherparameter) in which both of the arguments are of the same type, are prone to errors with the parameters swapped.

@yevhenii-nadtochii
Copy link
Copy Markdown
Contributor Author

@armiol PTAL

@yevhenii-nadtochii yevhenii-nadtochii merged commit 976d3bf into master Apr 29, 2025
7 checks passed
@yevhenii-nadtochii yevhenii-nadtochii deleted the report-illegal-placeholders branch April 29, 2025 09:44
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