Skip to content

Add subject field to all *OptionDiscovered events#347

Merged
yevhenii-nadtochii merged 7 commits intomasterfrom
subject-option-events
Apr 15, 2025
Merged

Add subject field to all *OptionDiscovered events#347
yevhenii-nadtochii merged 7 commits intomasterfrom
subject-option-events

Conversation

@yevhenii-nadtochii
Copy link
Contributor

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

This PR adds subject field to all *OptionDiscovered events.

Having this field instead of just FieldName, TypeName and so on, helps to validate the option right in Policy upon the *OptionDiscovered event. Otherwise, we have to collect the required data in a view and only then validate. And the view doesn't know whether a member is marked with the option in advance.

Currently, we already have subject for FieldOptionDiscovered and OneofOptionDiscovered events. This change set suggests propagating this property to all similar events:

  • TypeOptionDiscovered (replace type: TypeNamesubject: MessageType)
  • EnumOptionDiscovered (replace type: TypeNamesubject: EnumType)
  • EnumConstantOptionDiscovered (replace type: TypeName, constant: ConstantNamesubject: EnumConstant)
  • ServiceOptionDiscovered (replace service: ServiceNamesubject: Service)
  • RpcOptionDiscovered (replace service: ServiceName, rpc: RpcNamesubject: Rpc)

The only exception is FileOptionDiscovered, which will continue to have just file: File for the following reasons:

  1. The file: File property is requested by FileAware interface, which is implemented by everyast event.
  2. Having a property with the same value under a different name subject: File has no sense.

Additionally, I suggest renaming of TypeOptionDiscovered to MessageOptionDiscovered. A type can be a message or enum, but we already have EnumOptionDiscovered event for that.

@yevhenii-nadtochii yevhenii-nadtochii self-assigned this Apr 14, 2025
Base automatically changed from fix-oneof-span to master April 14, 2025 16:16
@codecov
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 74.82%. Comparing base (a928b76) to head (2d6789e).
Report is 8 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #347      +/-   ##
============================================
+ Coverage     74.81%   74.82%   +0.01%     
  Complexity      615      615              
============================================
  Files           196      196              
  Lines          4256     4254       -2     
  Branches        396      396              
============================================
- Hits           3184     3183       -1     
+ Misses          939      938       -1     
  Partials        133      133              
🚀 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 15, 2025 13:44
@yevhenii-nadtochii
Copy link
Contributor Author

@armiol @alexander-yevsyukov PTAL

Copy link
Collaborator

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

LGTM

@yevhenii-nadtochii yevhenii-nadtochii merged commit 6f966a5 into master Apr 15, 2025
8 checks passed
@yevhenii-nadtochii yevhenii-nadtochii deleted the subject-option-events branch April 15, 2025 14:51
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.

3 participants