search

LaurentTreguier / vscode-uncrustify

Code format using uncrustify
66 stars 10 forks source link

fix formatter to work properly with range formatting and document for… #12

Closed everLEEst closed 7 years ago

everLEEst commented 7 years ago

…matting both

  1. document.getText have override functions non-parameter and one-parameter, so change code to set range and non-range case properly.

  2. Adding new child process will return new pid 1, so to prevent error message show in window popup, change error checking condition to less then zero(which means real error case in waitpid()).

I've checked code works well in my local vscode, but if this fix is wrong, please revert it.

Signed-off-by: SangHyeon Jade Lee dltkdgus1764@gmail.com

LaurentTreguier commented 7 years ago

I don't think the document.getText(range) needs to be changed... If there is no range then range will simply be null, it will be ignored, and everything seemed to work properly like that. But the error code checking sure is useful, I'll merge this and push a new version later today when I can.

everLEEst commented 7 years ago

actually for my vscode, the uncrusitfy feature is broken without this patch, so that is why I pull request this change. https://code.visualstudio.com/docs/extensionAPI/vscode-api#Range the document looks, getText overload two functions, one without parameter, one with parameter range. not sure how it implemented in vscode, but at least, we couldn't sure getText() == getText(null) and actually for me, it looks not working like that equal. I guess when parameter is not comming they just get full range of document, but give null, anyway we set a value to range, so it just understand range is null. not sure about this... just for me it looks working like that and... fixing that line, also make everything working well in my vscode formatting.

If current code works well in your vscode then, it's fine, I can modify my extension locally, but please check it.

Thank to your fast answer and review. I'm fine about reviewing or version up with delaying :) any time you available, please check them. thank you.

LaurentTreguier commented 7 years ago

If it doesn't work with your VSCode, then this patch is indeed necessary. What's your VSCode version and platform?

everLEEst commented 7 years ago

VSCode 1.14.2 the latest version and linux platfrom(ubuntu 16.04) I can try in linuxmint.

LaurentTreguier commented 7 years ago

That's weird... I have version 1.14.2 as well, on Fedora 26, so it should be about the same... Anyway, I won't take any chances

everLEEst commented 7 years ago

sorry for wrong infomation, yes your previous code is right and the range condition thing is redundant. the original problem is actaully in uncrustify create again, sorry,

I using path to set a config like {PATH_ROOT}/uncrustify.cfg

when I process uncrustify.create, it re-wrote my pathes config file, so my path config file lost every config declair and again, the uncrustify.cfg file to be empty again.

that is why it works weirdly. sorry for wrong notification friend :( I using my own path so there are no change to using create now, but I looks still buggy.. and I think the file need to be created in parent directory of editing source, not the path..

anyway you can just revert my patch and keep error things only.. your first comment was correct :) thank you.

LaurentTreguier commented 7 years ago

I don't understand what doesn't work exactly... If you use uncrustify.create, of course it should create a new fresh default config (and thus erase the old one if it exists). Maybe I should add a warning if a file already exists though... If you would like to use a new config separate from {PATH_ROOT}/uncrustify.cfg, you can use a local .vscode/settings.json file to set the path to null so that it defaults to the workspace root.