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

Added support for rangeFormatting #120

Closed
wants to merge 6 commits into from
Closed

Conversation

ryuukk
Copy link
Contributor

@ryuukk ryuukk commented Mar 8, 2021

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the addition. This is quite a complex change and also changes how the previous full formatting works.

Because of this, this PR should really add some unittests for the new diff method.

source/served/commands/format.d Outdated Show resolved Hide resolved
{
//dfmt off
return (editRange.start.line < formatRange.end.line
|| (editRange.start.line == formatRange.end.line && editRange.start.character <= formatRange.end.character))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add compare functions / operators for Position so this is easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how i should write this either xD

I think it's best to keep this simple, it's not needed elsewhere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the struct Position add:

    int opCmp(const Position other) const
    {
        if (line < other.line)
            return -1;
        if (line > other.line)
            return 1;
        if (character < other.character)
            return -1;
        if (character > other.character)
            return 1;
        return 0;
    }

this allows you to compare positions like a < b

I believe it would be simpler to just have

editRange.start <= formatRange.end && editRange.end >= formatRange.start

here.

@ryuukk
Copy link
Contributor Author

ryuukk commented Mar 9, 2021

I can make the "formatting" untouched, and only "rangeFormatting" uses the new diff method, so it doesn't change the all behavior at all

Also i contacted DLS author about taking some of their ideas, he agreed about shipping the code from GPL3 to MIT

LaurentTreguier/azure-dlang-test#1

@WebFreak001
Copy link
Member

I think it's ok for the full formatting method to use diff as well (it improves bandwidth), just needs some tests

@ryuukk
Copy link
Contributor Author

ryuukk commented Mar 14, 2021

I'm not sure how i should write the tests

@WebFreak001
Copy link
Member

all structs are plain old data. You can simply add a unittest block after the method and call it with your custom test data and run the tests using dub test.

@ryuukk
Copy link
Contributor Author

ryuukk commented Mar 14, 2021

I reverted the old behavior, because i don't know exactly what to do with tests, i don't want to mess up things that were working previously, i'm not familiar enough with the project

If someone wants to change the old behavior, it could be done on a separate PR

@WebFreak001
Copy link
Member

ok I have added a few unittests, the diff algorithm really does work amazingly for how simple it is! I'm squashing it and merging it in directly on master + my changes afterwards, so I'll close this once done. (with reference to merge commit)

@WebFreak001
Copy link
Member

merged in with 95c14e9

@WebFreak001
Copy link
Member

here are some commits you may be interested in checking out what I changed in your code:

  • 303e261 - added tests in this commit
  • 2a65f69 - fix format diff for utf32 characters
  • f830dca - reduce complexity of format diff for large files

you initially used offsetToPosition (which is UTF-16 codepoints) instead of bytesToPosition, which caused bugs with non-ascii code, so I changed that and then optimized using the movePositionBytes which reuses calculation of previous positions, which is essential for big files.

It would be possible to actually include the position calculation inside the diff code, as then it wouldn't need to iterate over the whole file twice, but I think it's ok how it is now.

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

Successfully merging this pull request may close these issues.

Enhancement: format selection
2 participants