-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Detect deprecated View file types #320
Conversation
* viewType deprecations * routing/targets deprecations
@@ -175,6 +178,25 @@ export const MESSAGE_INFO = { | |||
details: ({details}: {details: string}) => details, | |||
}, | |||
|
|||
[MESSAGE.DEPRECATED_VIEW_CONFIG]: { | |||
severity: LintMessageSeverity.Error, | |||
ruleId: RULES["no-deprecated-api"], |
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.
I know we're trying to consolidate those, but I'm still wondering whether we need to distinguish between API deprecation, partial deprecation and whole files/patterns deprecation...
Just food for thought and something we might want to discuss.
My point is that in the future we might want to do some filtering based on ruleId and no-deprecated-api
would have a significant percentage of the findings
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.
Yes, absolutely. Similarly in #316
I find it hard to come up with rule names that generalize some of our messages without getting too specific again. Ultimately most of these findings are reported because something got "deprecated", but it's not really an API. So "no-deprecated-view-type"? That's already almost too specific and can only cover few of the messages.
We should look at the whole list we have now and re-group things ideally before the 1.0 release.
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.
FW, FV and me discussed this briefly. Looking at it from an app developer perspective it seems reasonable to collect all deprecations under a single (or very few) rules. We couldn't identify any benefit from differentiating between "deprecated views", "deprecated properties", "deprecated parameters" etc.
Ultimately a developer will have to deal with all of these cases very similarly.
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.
LGTM with some small remarks
src/linter/fileTypes/linter.ts
Outdated
]; | ||
|
||
export default async function lintFileTypes({workspace, context}: LinterParameters) { | ||
let xmlResources: Resource[]; |
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.
To me this var name seems a bit ambiguous as it refers to XMLResources, but what we're looking for is anything, but XML :D
Maybe, potentialDeprecatedResources
or something in that direction
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.
Done
src/linter/fileTypes/linter.ts
Outdated
]; | ||
|
||
export default async function lintFileTypes({workspace, context}: LinterParameters) { | ||
let xmlResources: Resource[]; |
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.
To me this var name seems a bit ambiguous as it refers to XMLResources, but what we're looking for is anything, but XML :D
Maybe, potentialDeprecatedResources
or something in that direction
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.
Duplicate comment.
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.
LGTM
JIRA: CPOUI5FOUNDATION-883