WIP: Issue #640

[protiumx]

Notes:

• The code works as expected but I need some definitions to finish this.
• Using os.hostname() instead of sync.host
• First time I read code for a VS Code extension. I based my implementation only by reading the existing code.
• I made tests only in Linux (Arch Linux x64, VS Code 1.27.2)

As specified, initially I made 2 regex patterns:
/\/\/\s\@sync\s(os=(\w+)\s?)?(host=(\w+)\s?)?\n(.+),?/g
/\/\/\s\@sync\signore\n.+,?/g

I wrote test files separated from the source code because I didn't see any test folder. I didn't find any reference in contributing guide.

Questions:

1. Wouldn't be better if instead of adding comments we remove the lines that doesn't match host or os? Thus, users will have in their local file only the settings that match the pragmas statements.
2. Should this implementation support multiple white spaces or line breaks?
3. It's okay this array of OS? ["windows", "linux", "mac", "darwin"]. I think we should only use darwin instead of mac

[shanalikhan]

Issue #640 initial implementation

Thanks for your work.
initial implementation is confusing, If it is work in progress, Please change the title to WIP : #640

I make test files separated from my fork because I didn't see any test folder. I didn't find any reference in contributing guide....I make 2 regex patterns

Yes there are no test cases for now, it would be great if you add them also by creating the test folder as per code extension standards, so it would easy for me to evaluate and publish easily.

Settings Sync needs test cases and I'm always looking forward to add them but due to time constraints i couldn't

Questions:

1. Understanding Host Functionality

Using os.hostname() instead of sync.host

By the host meaning, I mean tht environment name actaully. There are two things if you look into the comment: #640 (comment)
Thats why putting this key as a Sync Global Setting in dare need.

// @sync os=windows host=home

User can have Office and Home envionment if single GIST with specified OS

For example: User have Mac in both Home and Office, they might have different settings managed.
We can use the OS list that is already present in Settings Sync as so far i havent received any issue with the OS list.

2. Adding Comment or Removing

Wouldn't be better if instead of adding comments we remove the lines

Adding comment will be good option if user wants to see the office environment setting and want to use those settings as a part of Home environment host.

3. Line Breaks

Should this implementation support multiple white spaces or line breaks?

Sorry i dont understand, Would you please explain it.

Thanks ❇️ for your work so far !
Settings Sync becoming better and better !

[protiumx]

Sorry i dont understand, Would you please explain it.

Take as example

{
  // @sync        os=mac           host=Laptop --multiple white spaces or tabs
  windows.zoom: 1,

  // @sync os=windows --multiple line breaks after pragma. I think it should be only one line break strictly 

 windows.zoom: 2,
  ...
}

To leave it clear:

• OS and HOST values are case insensitive?
• OS list is: [linux, mac, windows]. Any other value will be ignored
• HOST can contain especial characters like underscore or hyphens? This is for calibrate the regexps
• The ignore pragma should be @sync ignore or @sync-ignore?
• /* */ comments are valid?

I will write tests using BDD pattern. I was thinking in jest or mocha. Let me know if you have any preference.

Thats why putting this key as a Sync Global Setting in dare need

Is it already loaded in the code or I had to load the Sync Global settings file?
What is the expected behavior if the host value is undefined, null or empty?

The TODO list:

• Write tests for this feature for every pragma statement combination
• Update the wiki with the info defined here

Ideas:

• If the user can have multiple machines with the same host name and os we can add an env pragma. E.g:
  // @sync hots=Laptop env=ESPECIAL_CASE where ESPECIAL_CASE comes from export ESPECIAL_CASE=1. The value is ignored, it only matters if process.env contains that key. This could be a further implementation in case you find it useful.

[shanalikhan]

Yes one strict line break should be supported.

OS and host should be case insensitive.
Host name support should have special character support. For now only those key OS support is needed.
Ignore pragma should be @sync-ignore but if it finds @sync ignore it should change it to @sync-ignore to assis user.

Can u explain about the validity of comment u have asked?

Please use mocha for writing test cases. Also integrate running those test cases with build if possible to steam line the processes via npm build.

Settings Sync global settings always load on download or upload
Global setting json file contain user token. You can check how it is reading the token from.the global setting file.

If host config is null or undefined it should only all the os settings that are

@sync os=windows

And general settings

It should also apply any if the @sync os=windows host=abc

Please Make sure while uploading it should verify the user have written correct os name or host name- validation checks.

Yes checking key from process.env is also good option that we can implement.

Feel free to ask questions!

[protiumx]

Cool, so, to sum this up:

• Check for @sync ignore and change it to @sync-ignore
• Check OS name. Alert user if OS doesn't match to any OS or it contains spaces or tabs after the = Same for host name.
• Get host from customSettings.hostName
• Implement env options
• Tests with mocha and integrated to the build process.

Another thing would be great if a setting can be applied to multiple host or OS. E.g.

// @sync host=laptop1|laptop2|pc1 os=linux|mac

and last, it would be great to have a prompt with all the pragma settings applied to the current host.
But I don't want to extend this pull request. So if you find it useful you can make another feature request and I will work on that (or some other contributor)

I will let you know when this is ready!

[shanalikhan]

Perfect!
Please note that host is in global settings where token is available. (Not sync.host) readme has details on global settings

Yes that is great but now let's keep it outside from this PR scope.

[protiumx]

@shanalikhan I think I finished. Could you give me feedback?
Notes:

• I added some reference to the new feature on the README.md.
• Before upload I remove white spaces from values. E.g. os= linux to os=linux
• Before upload I remove all the @sync-ignore and @sync ignore lines.
• os, host and env can be in any order
• I write some expects, could you check them?
• I added a test script. But the project has an error running the compile script. I didn't fix that because is out of this PR

The key of this feature is that before upload the settings we make sure that it only contains valid pragmas. Then when settings are downloaded, we process those pragmas and insert comments if we have to.

The code can accept any string. So when #258 is ready we can support any custom file with @sync pragmas. It only has to match. For this PR it only modify the settings.json content

When this PR is merged we could implement OR operator on each pragma. E.g.: @sync env=WORK|HOME os=linux host=laptop|pc1

[protiumx]

@shanalikhan about my last commit.
Should we uncomment all settings before upload and only comment settings before write the file into the disk? I will wait for your feedback about the lasts commits to finally close this PR. Here is all the relevant code to this PR pragmUtil.ts

Regards

[shanalikhan]

Should we uncomment all settings before upload and only comment settings before write the file into the disk

Yes thats perfect !

Public Wiki edit is enabled. When this PR is complete and you have time. It would be great if you can create a separate page over there and display the work details.

Thanks.

[protiumx]

I will add a wiki page for this.
I wrote test only to check the the PragmaUtil expected outputs and not for the integration with the whole extension. I think the only way will be writing a test that forces upload/download to check if it fails some where.

Could you check that? I think this is ready to merge. Let me know to close this PR

[shanalikhan]

Thanks @ioprotium !
Its merged.
The only thing left is the documentation page in the Wiki.
I will release new version in a week or two.

Feel free to improve where you think it can :)

[protiumx]

You are welcome! It's so cool to get involved in open source projects that I use a lot!
I edited this wiki page.
On my lasts commits I improved the code a little bit. Any issue related with this feature please assign it me to me.

[connorshea]

I was just looking for a feature like this, thanks for implementing it @ioprotium!