Feature: Prettier plugin #1540

akinsho · 2018-02-12T23:24:14Z

As discussed on Gitter, this is a plugin to provide prettier's autoformat capabilities to Oni.

Todo

Status bar item ~~with prettier logo~~ - stretch goals: show cross if formatting failed- At present cannot include images ?webpack setup?
Check for prettierrc and give that priority if present
Setup user config for prettier, should match the prettier config opts settings
Fix cursor positioning on format
Save buffer following format otherwise buffer saves autoformats aka buffer is continuously in modified state.
Tests using @TalAmuyal's plugin testing example for Markdown preview

codecov · 2018-02-12T23:42:27Z

Codecov Report

Merging #1540 into master will increase coverage by 0.08%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1540      +/-   ##
==========================================
+ Coverage   35.76%   35.85%   +0.08%     
==========================================
  Files         278      279       +1     
  Lines       10767    10800      +33     
  Branches     1350     1350              
==========================================
+ Hits         3851     3872      +21     
- Misses       6732     6744      +12     
  Partials      184      184

Impacted Files	Coverage Δ
...src/Services/Configuration/DefaultConfiguration.ts	`81.25% <ø> (ø)`	⬆️
browser/src/Editor/BufferManager.ts	`8.12% <0%> (-0.22%)`	⬇️
...vices/Learning/Tutorial/Stages/MoveToGoalStage.tsx	`41.93% <0%> (-1.4%)`	⬇️
browser/src/Services/Learning/Tutorial/Notes.tsx	`55% <0%> (-0.77%)`	⬇️
...src/Services/Learning/Tutorial/Tutorials/index.tsx	`100% <0%> (ø)`	⬆️
...ning/Tutorial/Tutorials/SearchInBufferTutorial.tsx	`84.21% <0%> (ø)`
browser/src/Services/Sneak/index.tsx	`28.57% <0%> (+0.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86ced53...110070a. Read the comment docs.

add initial attempt at cursor position

TalAmuyal · 2018-02-13T14:37:16Z

Seems like a serious endeavor, good luck!

akinsho · 2018-02-13T22:10:38Z

@bryphe might be getting carried away but thought it would be cool to use a small Icon of prettier's logo in the status bar item which I have added which I plan on adding an on click to format functionality and showing a cross (font awesome) if an error occurs, I dont believe the webpack config is setup to use assets? or would you ideally rather avoid that its a very minor point just seemed like a cool idea at the time.

One issue that making plugin(s) has raised is that by using react from Oni as a dependency you cant use JSX, styling with styled-components etc (this is more of a general question than specific to this plugin) I wonder if there's been discussion around that. I know @TalAmuyal imported react as a separate dependency for the markdown plugin just curious

bryphe · 2018-02-14T17:33:35Z

vim/core/oni-plugin-prettier/index.js

+        }
+    }
+
+    // TODO check for prettierrc and use that if present


bryphe · 2018-02-14T17:34:21Z

vim/core/oni-plugin-prettier/index.js

+    // TODO check for prettierrc and use that if present
+
+    Oni.editors.activeEditor.onBufferSaved.subscribe(async buffer => {
+        if (config.formatOnSave && config.enabled) {


Do we need to check the buffer types here? Or is it gated somewhere else? Just wondering how its kept from running on files that might not be applicable

That's definitely a concern so need to do that as you point out 👍 , just stalled on this trying to figure out how to use the cursor offset digit prettier gives us to generate a line and column number as the function does not return a line and column number which is what oni expects just a single number I assume is the index where the cursor was

bryphe · 2018-02-14T17:34:48Z

vim/core/oni-plugin-prettier/index.js

+    })
+
+    async function applyPrettier(buffer = currentBuffer) {
+        const arrayOfLines = await currentBuffer.getLines()


Awesome, this looks nice!

more work on cursor positioning add success and error status icons

add allowedFiletypes option to config

akinsho · 2018-03-05T18:47:37Z

@bryphe I've completely stalled on this as I got the actual formatting functionality working with an associated status bar item which you can click to format and also shows success and error indicators shortly after the initial suggestion on gitter but I'm stuck trying to get the cursor to return to the exact position it was left at as prettier has a formatWithCursor option.

The method returns just a number which is the offset in the bufferString, converting that to and from oni's line and column has been quite tricky and whilst atm its seems to return to the right place about 60% of the time I wonder how much of a blocker to integrating this you think that will be, it wasn't part of the initial plan but I got ambitious/greedy once I read the docs on that method.

badosu · 2018-03-06T12:56:34Z

@Akin909 Will this plugin be enabled by default?

akinsho · 2018-03-06T13:34:17Z

@badosu no definitely not it will be opt-in unless @bryphe disagrees, code formatting is way to controversial an issue for this to be on by default imho, I've added a default config setting which sets it to off so a user will have to enable it to get the functionality.

akinsho · 2018-03-06T13:38:42Z

@TalAmuyal any chance I could pick your brain re how to use the plugin testing strategy you employed for the markdown-preview-plugin not entirely clear on how to use it

badosu · 2018-03-06T13:39:31Z

Thanks @Akin909, we need to provide some instructions on the wiki so that I and others can check it out when it get's released to the stable channel!

akinsho · 2018-03-06T13:41:26Z

Will write up some documentation for it once it's closer to being releasable, still need to try and add some tests for it 😟 and take one last stab at returning the cursor to the right position post formatting.

badosu · 2018-03-06T13:48:17Z

You're doing a great job @Akin909! Thanks!

bryphe · 2018-03-06T19:01:56Z

You're doing a great job @Akin909! Thanks!

💯 to that! Thanks @Akin909 for all you're contributing 😄

bryphe · 2018-03-06T19:04:12Z

@badosu no definitely not it will be opt-in unless @bryphe disagrees, code formatting is way to controversial an issue for this to be on by default imho, I've added a default config setting which sets it to off so a user will have to enable it to get the functionality.

👍 , it should be off-by-default for now. It might even make sense to have it as a separate plugin at some point (once we have a plugin-install-story).

bryphe · 2018-03-06T19:11:13Z

The method returns just a number which is the offset in the bufferString, converting that to and from oni's line and column has been quite tricky and whilst atm its seems to return to the right place about 60% of the time I wonder how much of a blocker to integrating this you think that will be, it wasn't part of the initial plan but I got ambitious/greedy once I read the docs on that method.

There's a 'Vim' way to do this. We even send up this 'byte offset' in as part of our event context:

oni/vim/core/oni-core-interop/plugin/init.vim

Line 176 in e47434d

let context.byte = line2byte(line(".")) + col(".")

It also has an inverse byte2line method to go from a byte (offset) to a line - and then you can use that to derive the column too with the line2byte method. I think it would make sense to include this as a function on our Buffer API - we have getCursorPosition today which returns a {line, column}, but it would be good to have getCursorOffset which returns {byteOffset: number}, since some API/tools prefer that. Under the hood, we could use those VimL methods as described. Let me know if that makes sense or you have questions-

add new [wip] ci test

make convertCursorPos zero base to march getCursorPos

akinsho · 2018-04-01T14:53:39Z

@bryphe I dug into the highlighting issue a bit and noted a few things.

The highlighting behaviour is sporadic as sometimes the highlighting remains unchanged.
Sometimes the line that the cursor is on remains highlighted but the lines around it lose their highlighting.
Logging shows that an updateBuffer event is fired. Not sure whether when set lines is called a full re-highlight is triggered or not, come to think of it not sure when highlighting is applied.

I'm noting these issues here but ideally feel like they should be tracked and resolved relating to #1838 rather than here

PS: Added some CITests (for the new buffer methods and the plugin itself)

move failing windows test to bottom to see if only that test fails

akinsho · 2018-04-03T09:25:33Z

@CrossR @bryphe if either of you get a moment I'd really appreciate it if you could try running this branch locally, you will need to enable the plugin via the config option.

	"oni.plugins.prettier": {
		enabled: true,
	}

If you open a .ts file make a change and save I'm trying to figure out if the prettier module is failing for some unseen reason on windows (the ciTests for windows are failing on the prettier test but not on linux and macOS), I'm trying to get a vm setup to do this myself but till then It'd be really helpful to know if some error is present that I can start to look at

CrossR · 2018-04-03T10:37:25Z

Hey @Akin909, I've given it a try and I haven't noticed any errors yet, but I also haven't noticed it changing the code as I'd expect.

Just to check I'm not missing something:

Pull/Install/Build/Run (I did update the package.json have the plugins:watch command watch the prettier plugin as well)
Added the following to my config (taken from the CI test):

        "oni.plugins.prettier": {
            settings: {
                semi: false,
                tabWidth: 2,
                useTabs: false,
                singleQuote: false,
                trailingComma: "es5",
                bracketSpacing: true,
                jsxBracketSameLine: false,
                arrowParens: "avoid",
                printWidth: 80,
            },
            formatOnSave: true,
            enabled: true,
            allowedFiletypes: [".js", ".jsx", ".ts", ".tsx", ".md", ".html", ".json", ".graphql"],
        },
    },

Made a file called oni.ts. Once opened I see the prettifier status icon.
Added function test(){command.log('test')};
Hit :w and can see the prettifier being run if I debug the onSave subscription.

But after that point, I don't see anything change in the code, at least the semi-colon remains and what not. Is there anything I'm missing/could do to help?

One odd thing I did notice was that it seemed to run repeatedly for me. That is, I saved, it ran on save, at the end of the updating, it then called :w which caused the on save to be run again, which ran the prettifier which saved and so on.

Oh, and if I debug I can see the code has been prettified, its just not being set for some reason. Its possible a line ending issue since arrayOfLines.length is 1 despite the pretty code being:

function () {
    console.log("test")
}

Which I assume should be 3 lines?

EDIT: Looks like its running repeatedly since the lastBufferState is set to the pretty code, whereas the buffer isn't updated so the if fails. So that is only due to the buffer failing to be set.

CrossR · 2018-04-03T10:50:48Z

This what I currently see in the setLines call, perhaps that helps:

And as much of the rest of the stuff as I can fit on screen at once:

akinsho · 2018-04-03T12:01:49Z

@CrossR thanks soo much for testing that out and for the detailed feedback that really helps 👍.

I'm essentially using 0, arrayOfLines.length as the boundaries for the text to be replaced so think that works as expected (the array actually comes from Oni so think it's getting split properly).

But it does point out an assumption I made which is that prettier would return the code with platform specific newlines but actually it seems like it might always return a \n so the split never works so the code isn't replaced properly I think.

CrossR · 2018-04-03T12:37:22Z

That seems to have fixed it for me!

Not fully sure about the different syntax highlighting there for test vs test2, but the format worked.

akinsho · 2018-04-03T12:44:57Z

@CrossR 💯 re the syntax highlighting tbh I don't know enough about how it's applied to explain why it gets messed up somehow calling setLines doesn't trigger a re-application of the syntax highlighting 😭 raised it as a separate issue but tbh not sure where to even look or what might be wrong.

Seems a buffer update event is being triggered but I think the current strategy only tries to re-highlight conservatively 🤷‍♂️ not sure

CrossR · 2018-04-03T12:53:20Z

Thought it was a known issue, so at least it can be sorted in a different PR.
Cool that its working though, CI test and all!

Slight tangent, but is it possible for us (ie not @bryphe) to restart tests on AppVeyor? On Travis I see a run-build button when I'm logged in with my GitHub account, but this isn't the case for AppVeyor, which would be useful when you get errors like you have with something seemingly entirely unrelated.

slightly excessive for now but a foundation for further tweaks in the ongoing development of this plugin

switch to function keyword to ensure applyprettier is hoisted so callback can reference it

akinsho · 2018-04-04T08:21:08Z

@CrossR thanks 👍, re the AppVeyor test nope I don't get the option to re-run things would actually be really helpful (tbh I didnt have an account, I just created one and had a look but no option to rerun tests appeared)

akinsho · 2018-04-06T00:27:36Z

@bryphe not sure why the last check is failing but this is done now and shouldn't have anymore syntax hiccups (tested locally).

bryphe · 2018-04-06T00:47:34Z

Awesome! Looks like the build is good now too 👍

Slight tangent, but is it possible for us (ie not @bryphe) to restart tests on AppVeyor?

Ah interesting, I thought it would be like Travis - was hoping at least collaborators would be able to restart the build! It does seem like I can give permissions - but the annoying thing is its not connected with github for that. @CrossR , I had your e-mail address from the OpenCollective, so I added you - but @Akin909 , I didn't have yours handy - would you mind sending it to me on twitter/discord? I'll add you as well 👍

It would be awesome if you didn't need me to restart it, I feel bad being the bottleneck 😄

Change looks good to me, glad you got the CiTest working @Akin909 ! Thanks for your work on this!

badosu · 2018-04-06T07:14:11Z

@bryphe @Akin909 Amazing!!

I'd like to try it, is there documentation on how to setup and how it works somewhere?

Add initial implementation of prettier plugin

fbdc9eb

akinsho added 5 commits February 12, 2018 23:46

add config options to test using config functionality

2ea7de0

update config to allow disabling, add status icon

57d0913

comment out console.log add check if code is formatted

3461897

add initial attempt at cursor position

fix config to camelcase

a37ee1e

add assets for prettier icon

865ab1d

bryphe reviewed Feb 14, 2018

View reviewed changes

vim/core/oni-plugin-prettier/index.js Outdated

}

}

// TODO check for prettierrc and use that if present

Copy link

Member

bryphe Feb 14, 2018

Choose a reason for hiding this comment

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

👍

bryphe reviewed Feb 14, 2018

View reviewed changes

akinsho added 6 commits February 14, 2018 18:09

add line column and [wip] attempt to return correct cursor pos

0d972e9

fix cursor positioning

626af41

Remove linecolumn, not working remove prettier asset

61ac068

more work on cursor positioning add success and error status icons

update yarn lock

32641cd

Fix eslintrc add allowedFiletypes checking

db9e987

add onClick fn to prettier element

afab171

add allowedFiletypes option to config

akinsho added 2 commits March 6, 2018 13:36

merge upstream fix conflicts

1f69230

remove console.logs

005e4e5

akinsho added 7 commits April 1, 2018 11:55

ensure existence of prettier status item

df1a7bf

add helper functions to common

8cb589b

add new [wip] ci test

merge upstream fix conflicts

0bcbba8

refactor get cursor offset to a get method

6436412

make convertCursorPos zero base to march getCursorPos

add new bufferCursorTest file

0f5fd71

fix missing CI test and remove redundant calc

d597049

remove redundant methods from prettier test

8c62ec6

tweak function to keep state within function rather than globally

06c9163

move failing windows test to bottom to see if only that test fails

remove specific use of newline per platform

cd3a4b2

akinsho added 2 commits April 4, 2018 05:20

switch to a more functional approach

2e12c86

slightly excessive for now but a foundation for further tweaks in the ongoing development of this plugin

fix prettier callback to use new state tracking version

5a22fe5

switch to function keyword to ensure applyprettier is hoisted so callback can reference it

akinsho added 4 commits April 4, 2018 13:41

fix typo and add check in prettier rc function

5f8a971

use prettier info to poplulate compatible filetypes

0039a0f

fix incorrect ref to config

3374972

actually remove incorrect conffig ref

110070a

bryphe approved these changes Apr 6, 2018

View reviewed changes

akinsho merged commit 3d21a97 into onivim:master Apr 6, 2018

akinsho deleted the feature/prettier-plugin branch April 6, 2018 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Prettier plugin #1540

Feature: Prettier plugin #1540

akinsho commented Feb 12, 2018 •

edited

Loading

codecov bot commented Feb 12, 2018 •

edited

Loading

TalAmuyal commented Feb 13, 2018

akinsho commented Feb 13, 2018 •

edited

Loading

bryphe Feb 14, 2018

bryphe Feb 14, 2018

akinsho Feb 14, 2018

bryphe Feb 14, 2018

akinsho commented Mar 5, 2018

badosu commented Mar 6, 2018

akinsho commented Mar 6, 2018

akinsho commented Mar 6, 2018

badosu commented Mar 6, 2018 •

edited

Loading

akinsho commented Mar 6, 2018

badosu commented Mar 6, 2018

bryphe commented Mar 6, 2018

bryphe commented Mar 6, 2018

bryphe commented Mar 6, 2018

akinsho commented Apr 1, 2018 •

edited

Loading

akinsho commented Apr 3, 2018

CrossR commented Apr 3, 2018 •

edited

Loading

CrossR commented Apr 3, 2018

akinsho commented Apr 3, 2018

CrossR commented Apr 3, 2018 •

edited

Loading

akinsho commented Apr 3, 2018

CrossR commented Apr 3, 2018

akinsho commented Apr 4, 2018 •

edited

Loading

akinsho commented Apr 6, 2018

bryphe commented Apr 6, 2018

badosu commented Apr 6, 2018

Feature: Prettier plugin #1540

Feature: Prettier plugin #1540

Conversation

akinsho commented Feb 12, 2018 • edited Loading

codecov bot commented Feb 12, 2018 • edited Loading

Codecov Report

TalAmuyal commented Feb 13, 2018

akinsho commented Feb 13, 2018 • edited Loading

bryphe Feb 14, 2018

Choose a reason for hiding this comment

bryphe Feb 14, 2018

Choose a reason for hiding this comment

akinsho Feb 14, 2018

Choose a reason for hiding this comment

bryphe Feb 14, 2018

Choose a reason for hiding this comment

akinsho commented Mar 5, 2018

badosu commented Mar 6, 2018

akinsho commented Mar 6, 2018

akinsho commented Mar 6, 2018

badosu commented Mar 6, 2018 • edited Loading

akinsho commented Mar 6, 2018

badosu commented Mar 6, 2018

bryphe commented Mar 6, 2018

bryphe commented Mar 6, 2018

bryphe commented Mar 6, 2018

akinsho commented Apr 1, 2018 • edited Loading

akinsho commented Apr 3, 2018

CrossR commented Apr 3, 2018 • edited Loading

CrossR commented Apr 3, 2018

akinsho commented Apr 3, 2018

CrossR commented Apr 3, 2018 • edited Loading

akinsho commented Apr 3, 2018

CrossR commented Apr 3, 2018

akinsho commented Apr 4, 2018 • edited Loading

akinsho commented Apr 6, 2018

bryphe commented Apr 6, 2018

badosu commented Apr 6, 2018

akinsho commented Feb 12, 2018 •

edited

Loading

codecov bot commented Feb 12, 2018 •

edited

Loading

akinsho commented Feb 13, 2018 •

edited

Loading

badosu commented Mar 6, 2018 •

edited

Loading

akinsho commented Apr 1, 2018 •

edited

Loading

CrossR commented Apr 3, 2018 •

edited

Loading

CrossR commented Apr 3, 2018 •

edited

Loading

akinsho commented Apr 4, 2018 •

edited

Loading