-
Notifications
You must be signed in to change notification settings - Fork 11
Update Recommendations.RecommendedActions.TargetFrameworks field. #51
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
base: master
Are you sure you want to change the base?
Conversation
The old `TargetFrameworks` schema is causing cta failures when parsing the rules. Also updated the indentation of a few recommendation files to be consistent with the rest.
I think we can ignore all the files with recommendation in their names as those were automatically added I believe and we have to revisit them at a later date anyway and they contain no rules. Some of the antlr rules file were heavily modified also, with a lot of redactions was that necessary to ensure they match the schema? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the changes look good, but the schema validator needs to be updated to match the updated schema.
- The schema validator is run as a GitHub action which we can see has failed here
- Code for the schema validator
Suggested changes:
- This property needs to be updated to match the schema in CTA
- Rename
RecommendationPOJO
toRecommendationPOCO
to follow C# naming convention
Jonathan made some good points... Not sure if the @Eruanion The heavy changes to the antlr rules only appear so due to changes in indentation. The content looks unchanged otherwise. |
These rule files are causing cta to fail though if we run it with
Yeah these files are indented with 2 spaces as the rest of them are 4 spaces, the content of the files are not changed. |
*.recommendation files shouldn't run in CTA, they don't have actions. We can modify each file if we have actions. To modify these files, we need to modify the code generating them since these are autogenerated. |
@marknfawaz Thank you for the explanation. Good to know the My only concern is the inconsistency of the |
Great catch @jonlouie , thanks for sharing the detailed information, if we do proceed with updating the |
@ShanshanHe These files are not relevant to CTA, but I don't see an issue with updating them. You can check with Andrew or Huawen on how they are being created. As for the change itself, while I agree it's good to keep all files in sync, we'll have to see how complex updating the file generation and validation process is then decide. |
Just had a chat with @flywanli and @huawenm , they are good with updating the schema. @huawenm has pointed me to the file generation code, looks like it'll be a very small change. And I have the validator updated as well in my local branch. Should I submit the change? |
The old
TargetFrameworks
schema is causing cta failures when parsing the rule files.Also updated the indentation of a few recommendation files to be consistent with the rest.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.