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

Support for comments when linting via added setting comment_symbol #34

Merged
merged 3 commits into from
Apr 25, 2019

Conversation

larsonmars
Copy link

This is a tiny fix for the linter warnings when there are comments at the beginning (see issue 14).

Copy link
Owner

@mechatroner mechatroner left a comment

Choose a reason for hiding this comment

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

Looks reasonable, but how about making it a little bit more generic?
Could you please change "comment_symbol" -> "comment_prefix" and adjust your code accordingly.

BTW, did you try to use Highlight extension with Rainbow CSV like it is described in this comment #14 (comment) ? If you did, please include your settings for "#" character into some place in the documentation.

extension.js Outdated
@@ -66,6 +66,8 @@ var global_state = null;

var preview_panel = null;

var comment_symbol ='#';
Copy link
Owner

Choose a reason for hiding this comment

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

need extra whitespace!

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no problem, I can fix this. Perhaps there should be some kind of linter configured, so this does not happen when coding. I really tried to adapt to the coding style. However, it took me half an hour to code and test the enhancement. It took me another hour to get all the trailing white space back again (see e.g. line 291, 410, 415, 528, ...) because my git and editor would remove it all the time and then the pull request contains many unrelated changes. This is also true for the README file.

Copy link
Owner

Choose a reason for hiding this comment

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

I am sorry for your troubles. But you could have fixed the trailing whitespaces and I would gladly accept that ;) BTW I think I have a linter config in the repo although I am not sure if it detects trailing spaces.
And trailing spaces in README is a feature, not a bug, otherwise github would concatenate different lines into a single long continuous paragraph - a very annoying behavior.

extension.js Outdated
if (!comment_symbol)
return document.lineAt(0).text;
const num_lines = document.lineCount;
for(let lnum = 0; lnum < num_lines; ++lnum) {
Copy link
Owner

Choose a reason for hiding this comment

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

need extra whitespace!

Copy link
Author

Choose a reason for hiding this comment

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

see above, will fix it.

@larsonmars
Copy link
Author

Regarding the highlight extension. I can have a look at it today at sometime. However as stated in the issue, the highlight extension does not fix the hover comments, which this enhancement is for.

@larsonmars larsonmars closed this Apr 25, 2019
@larsonmars larsonmars reopened this Apr 25, 2019
Copy link
Author

@larsonmars larsonmars left a comment

Choose a reason for hiding this comment

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

I think that should be it. The comment_symbol is now a comment_prefix supporting more tha one character as requested. I Also added something more to the Readme.

@mechatroner mechatroner merged commit 37fc7c6 into mechatroner:master Apr 25, 2019
@mechatroner
Copy link
Owner

Excellent! Thank you very much!
I want to add some more features before I publish a new version, stay tuned!

@mechatroner
Copy link
Owner

@larsonmars could you also add your *paderborn.de email to the github profile settings, because currently you are missing from contributors list. I think adding email should fix that ( see https://stackoverflow.com/a/43527384/2898283 )

@larsonmars
Copy link
Author

I'm happy that you like the addition. Maybe I will contribute more in the future. E.g., having the linting and white space issue solved. Also, there is room for improvement w.r.t. code duplication, but this is minor.

I have made my mail address public. Don't know if it changes anything.

Do I have to do sth. with this here to finalize it? When will the updated version be visible in VS Code?

Thank You!

@mechatroner
Copy link
Owner

The publishing is done separately through Visual Studio Marketplace system. I want to add more features before publishing a new version, see #35. If you want to implement linting - you are most welcome to do it (just make sure that your email in git config matches your email in github. Apparently registering it after the commit doesn't trigger github to link the commit to the account). I hope that the new release could be prepared within a month or so.

@mechatroner
Copy link
Owner

I just published a new version: 1.1.1 !
One issue though: after some considerations I decided to disable comment prefix by default. Users who are familiar with "comments in CSV" concept can enable it manually. For other users this will not cause problems in case when Align/Shrink or ColumnEdit commands won't behave as expected for some random lines. I can easily imagine a following CSV file, where skipping "comment lines" by one of these commands would be an error:

item_number,item_name,price
#100,foobar,$200
#40,fizzbuzz,$300

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.

2 participants