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

Add ending line and column to Location.kt #5032

Merged
merged 4 commits into from Jul 7, 2022

Conversation

VitalyVPinchuk
Copy link
Contributor

@VitalyVPinchuk VitalyVPinchuk commented Jul 3, 2022

Close #5025

Added endSource as the last parameter in the Location constructor.

I need some guidelines:

  • Is it necessary to change the name of parameters, for example startSource and endSource?
  • Should the order of the parameters be changed?
  • Should compact() string representation be updated?
  • Should there be a test for such trivia?

This could trigger changes in many files.

Copy link
Collaborator

@BraisGabin BraisGabin left a comment

  • Is it necessary to change the name of parameters, for example startSource and endSource?

I don't think so. That would introduce even more breaking changes in the api for little benefit.

  • Should the order of the parameters be changed?

That's up to you. You will need to create a new constructor matching the old one to avoid breaking the api so it's OK don't add it as the first one.

  • Should compact() string representation be updated?

No

  • Should there be a test for such trivia?

I don't think so. You are just adding a parameter.


But I would change the code inside core to inform this new value in this same pr. This way we can see that this is the change that we need.

detekt-api/api/detekt-api.api Show resolved Hide resolved
@VitalyVPinchuk
Copy link
Contributor Author

VitalyVPinchuk commented Jul 5, 2022

Hey @BraisGabin Thanks for clarifying things.
I made just another variable to data class and assign it in the new constructor.

@VitalyVPinchuk
Copy link
Contributor Author

VitalyVPinchuk commented Jul 5, 2022

But I would change the code inside core to inform this new value in this same pr. This way we can see that this is the change that we need.

Sorry, I'm afraid I didn't get it. What core are you talking about?

@BraisGabin
Copy link
Collaborator

BraisGabin commented Jul 6, 2022

But I would change the code inside core to inform this new value in this same pr. This way we can see that this is the change that we need.

Sorry, I'm afraid I didn't get it. What core are you talking about?

I was on my mobile and clearly I dind't explain myself.

You are adding the ability to store endSource inside Location.kt. What I think we should have in this same PR or at least in other PR on top of this one is the code that fills this information. The idea is to be sure that this API works for us. It should, but just to be sure.


Open question, mainly for maintainers. What do you think about a data class that has a property outside its constructor? (We could even have a rule for something like this). I don't know what to think about it. In first places Location sholdn't be a data class in fisrt place because it breaks the binary compatibility each time we change it. But now that it is a data class what do we prefer:

  • Ensure binary compatibility but with a "strange data class"
  • Keep the data class consistent but breaking the binary compatibility a bit
  • Remove data from Location to avoid future problems but with a bigger breaking change now

@VitalyVPinchuk
Copy link
Contributor Author

VitalyVPinchuk commented Jul 6, 2022

You are adding the ability to store endSource inside Location.kt. What I think we should have in this same PR or at least in other PR on top of this one is the code that fills this information. The idea is to be sure that this API works for us. It should, but just to be sure.

Now I see.
I modified from(element: PsiElement, offset: Int = 0) function so that it fills endSource.
I can also look for other places where constructors get called and try to fill new variable with appropriate value if possible.

@BraisGabin
Copy link
Collaborator

BraisGabin commented Jul 7, 2022

Thanks :) this looks great

@BraisGabin BraisGabin added this to the 1.22.0 milestone Jul 7, 2022
@schalkms schalkms merged commit 6638b2f into detekt:main Jul 7, 2022
17 of 18 checks passed
@schalkms
Copy link
Member

schalkms commented Jul 7, 2022

👍

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

Successfully merging this pull request may close these issues.

Add ending line and column to Location.kt
3 participants