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

How to override an existing check #8

Closed
PhilippSalvisberg opened this issue Dec 14, 2019 · 6 comments
Closed

How to override an existing check #8

PhilippSalvisberg opened this issue Dec 14, 2019 · 6 comments
Assignees
Labels
question Further information is requested

Comments

@PhilippSalvisberg
Copy link
Collaborator

This questions was asked by @S-Koell in another forum. Here's the relevant part:

As descussed here:

Trivadis/plsql-and-sql-coding-guidelines#78

I want to write my own validator based on the TrivadisGuidelines3Plus.xtend.

In my case I want to override the G-1050 but I'm really struggling because first of all I don't know what >the method is called to override. I was guessing "checkGuideline1050" because some with other >numbers exist. But 1050 doesnt.

Basically I want something like:

override checkGuideline1050(FunctionHeading f){
   if(NOT f.function.startsWith('logger')){
       super.checkGuideline1050(f);
   }
}```

Could you please give me guidance on how to approach this? 
@PhilippSalvisberg PhilippSalvisberg added the question Further information is requested label Dec 14, 2019
@PhilippSalvisberg PhilippSalvisberg self-assigned this Dec 14, 2019
@PhilippSalvisberg
Copy link
Collaborator Author

@S-Koell The guideline check G-1050 was already part of the Trivadis guidelines v2. Back then it was named G-05. Hence the check methods were also named checkGuideline5. There are two implemented one for string literals and one for number literals. I've created an example where I've re-implemented the guideline completely. See here. In your case you could also simplify the implementation to:

@Check
override checkGuideline5(SimpleExpressionStringValue literal) {
    if (!literal.isLoggerCall) {
        super.checkGuideline5(literal)
    }
}

It's a bit tricky to navigate the parse tree. I typically implement the code with the help of the debugger, which I call from a test.

@S-Koell
Copy link
Contributor

S-Koell commented Jan 16, 2020

Sorry for the late response but: WOW huge thanks for actually coding everything there!

But shouldn't it be:

        @Check
	override checkGuideline5(SimpleExpressionStringValue literal) {
		if ( !literal.isLoggerCall ) {
			super(literal); 
		}
	}

And I think this issue here is the right place to ask the development topics from the issue #10 :
How could I have known what to do there myself? So many questions from my side:

  • How would I find the correct method to override
  • With no idea what the classes are representing there is no way I could have coded that ( e.g. what is SimpleExpressionStringValue, BinaryCompoundExpressionLevel6 , ... )
  • Is there an Documentation for development etc.. ?

@PhilippSalvisberg
Copy link
Collaborator Author

No, super.checkGuideline5(literal) is correct. You just want to execute the standard behavior when the expression is not part of the logger call.

Regarding the other questions, I suggest to open dedicated issues. Thank you.

@S-Koell
Copy link
Contributor

S-Koell commented Jan 16, 2020

You just want to execute the standard behavior when the expression is not part of the logger call.

I agree.
But currently the code is:

if (!literal.isConstantDeclaration && !literal.isLoggerCall) {
			warning(1050, literal, literal)
		}

No call to super. That's why I asked if it's correct.

@PhilippSalvisberg
Copy link
Collaborator Author

PhilippSalvisberg commented Jan 16, 2020

The current implementation is not wrong. It just reimplements the validation completely. I wanted to make clear that reusing the existing implementation is also possible.

@S-Koell
Copy link
Contributor

S-Koell commented Jan 16, 2020

Understood. I thought that this could be the case but I couldn't be sure without knowing the default implementation, hence the question.

Thanks for clarification and a HUGE thanks for all the great and fast support you give!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants