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

Fix #686 #693

Merged
merged 27 commits into from
Oct 25, 2018
Merged

Fix #686 #693

merged 27 commits into from
Oct 25, 2018

Conversation

protiumx
Copy link
Contributor

@protiumx protiumx commented Oct 24, 2018

Extract all the pragma @sync-ignore lines and add them to the beginning of the downloaded settings.
It also fix an issue during the check of the settings as valid JSON.

Note: the settings are formatted with spaces or tabs. Should we normalize the text format or it is up to the user?

#686

Blackcatz1911 and others added 27 commits September 18, 2018 02:08
…sync pragmas. Should it support multiple spaces? Replacing settings.json content before writing file. Should be better removing lines instead of add comments? Replacing settings.json before upload to remove @sync ignore pragms.
…n every orther. Remove whitespaces before upload. Alert user if OS value is not a valid OS.
… Only insert comments if it doesn't match with matchine os or host or env. Uncomment line before write if it matched.
…ted. Get OS from OsType enum. Remove os.hostName()
…commas are removed. Must check this. If not valid OS is detected inform user. Added function to remove comments from text.
Get the local content and extract the ignored lines before writing the new settings. Add the ignored lines at the beginning of the settings object.
@shanalikhan
Copy link
Owner

Should we normalize the text format or it is up to the user?

By normalizing you mean, format the JSON, right? Yes if the JSON is validated we can normalise the text.

@shanalikhan shanalikhan added this to the v3.2.1 milestone Oct 24, 2018
@protiumx
Copy link
Contributor Author

I mean, if the text is indented with mixed tabs and spaces, we should upload/download the content indented only with tabs or only with spaces? This has no issues with the implementation, I asked because user can have the following format:

{
    setting1: true,
setting2:true,
              setting3:true
}

And if we normalize the indent to get a clean view of the settings it will be saved as

// two spaces ident
{
  setting1: true,
  setting2:true,
  setting3:true
}

My question is if that could be a feature of the sync extension or user should care if the settings are correctly indented or not, since they have to manually add the pragmas on the settings.json file rather than using the integrated settings view. VS Code has no issue parsing de JSON no matter how it is indented.

Anyway, this PR fix the bug with ignored settings. Maybe my question is out of the context of this bug. You can merge it if you don't see something to improve.

@protiumx protiumx closed this Oct 24, 2018
@shanalikhan shanalikhan reopened this Oct 25, 2018
@shanalikhan shanalikhan merged commit cf8eeb3 into shanalikhan:v3.2.1 Oct 25, 2018
@shanalikhan
Copy link
Owner

shanalikhan commented Oct 25, 2018

You can merge it if you don't see something to improve.

So i guess its ready, but i found its closed PR. i have reopened this and merged this.

Yes that would be a good improvement regarding normalization. When you have time feel free to improve where required.

Thank you for your contributions so far!

@protiumx
Copy link
Contributor Author

Yes! Sorry, still learning how to do PRs haha.
I was thinking, maybe users have comments or some custom things going on in theirs settings.json files. Maybe you should ask to the users if it would be something useful for them that code sync format the files or it is just irrelevant for them.

You are welcome! When I have free time I will look up for more improvements.

shanalikhan pushed a commit that referenced this pull request Nov 26, 2018
* Extract all the pragma @sync-ignore lines and add them to the beginning of the downloaded settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants