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

removeRange, replaceRangeBy with negative range duplicates that range #163

Open
lue-bird opened this issue Aug 12, 2023 · 2 comments
Open
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@lue-bird
Copy link
Contributor

lue-bird commented Aug 12, 2023

Describe the bug
When the start Location is later in the document than end,

  • removeRange duplicates the source code in the range with start and end inverted
    • example removing in inverted remove range: test remove testtest removeremove test
  • replaceRangeBy ... "A" is like removeRange with "A"` directly between the duplicated sources
    • example replacing by "A" in inverted replace range: test replace testtest replaceAreplace test

SSCCE (Short Self-Contained Correct Example)
https://github.com/lue-bird/elm-review-sscce-negative-range

Expected behavior
On the one hand it's good that you will notice that you've switched up start and end by having it duplicated; this exact behavior seems unintentional, though.

Possible alternatives are

  • whenever there's a negative range, fail fixing with a nice message
  • provide a fix which in the inverted range says something like "elm-review rule ERROR: start Location is later in the document than end"
  • silently do nothing in the fix

Additional context
Found this bug by accidentally messing up start and end locations.

@jfmengels
Copy link
Owner

I've noticed this problem as well before, and agree that this should not be the behavior. It's pretty cool to be able to duplicate code but I think it's rarely going to be useful the way it is now 😅

Whenever we try to apply fixes, it is either going to be successful or create a Problem, and that is our way of reporting issues. And I agree that that should be the way forward. Adding a new variant there would be a semver breaking change unfortunately (which I don't want to happen right now). So either we find a workaround, or we create a ProblemV2 and a FixResultV2 types.

Maybe I should have a | OtherProblem String variant in order to be able to create future kinds of problems without a braking change.

We could also ignore this fix altogether, but I think that is going to be confusing. I think we should probably go for the duplicate types for ProblemV2.

@jfmengels jfmengels added bug Something isn't working help wanted Extra attention is needed labels Aug 12, 2023
@lue-bird
Copy link
Contributor Author

lue-bird commented Aug 12, 2023

Yeah I'm sure we can find more edge-cases like this one in the future, with negative locations or non-existent locations etc, so adding an | OtherProblem String variant as well seems future-proof.
When some day v3 would come, all the found cases can then be explicitly added if it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants