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

@sync-ignore isn't ignoring my local value, it deletes it #686

Closed
borekb opened this issue Oct 18, 2018 · 4 comments
Closed

@sync-ignore isn't ignoring my local value, it deletes it #686

borekb opened this issue Oct 18, 2018 · 4 comments

Comments

@borekb
Copy link

borekb commented Oct 18, 2018

The latest 3.2.0 release implemented @sync-ignore pragma, see #640, but I don't think it's working as expected.

These were my steps to reproduce:

  1. Add // @sync-ignore before "window.zoomLevel": 1.
  2. Upload
  3. Download (forced download to be sure)

I would expect that my local settings.json still contains this:

    // @sync-ignore
    "window.zoomLevel": 1

but it didn't, these two lines were removed. Removing is not ignoring. In practice, this messes up my zoomLevel every time I invoke download which is unpleasant.

@shanalikhan
Copy link
Owner

@ioprotium can you look into it

@protiumx
Copy link
Contributor

protiumx commented Oct 18, 2018

@shanalikhan sure. The problem comes from the Sync settings initial behavior. When settings are downloaded it overwrites the entire file. We should compare the existent file and extract all the @sync-ignore in order to merge them with the downloaded settings before writing the file.

That shouldn't be difficult. But we need to define the following: when we merge the settings in order to keep the ignored ones, those will be

  • At the beginning of the file
  • At the end
  • Every ignored setting should keep its own line position // this will be a little tricky I think

What do you think? In my opinion, keep all the sync-ignored at the beginning of the file could be a good practice to make more readable the settings file.
To fix this should I pull from master or some other branch?

@shanalikhan
Copy link
Owner

shanalikhan commented Oct 18, 2018

In my opinion, keep all the sync-ignored at the beginning of the file could be a good practice to make more readable the settings file.

yes, thats perfect.

For PR, you can pull from master.

update : i have created branch v3.2.1

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

@shanalikhan Today I will start with this. Sorry for the delay, busy week at work.

@protiumx protiumx mentioned this issue Oct 24, 2018
shanalikhan pushed a commit that referenced this issue Oct 25, 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
Projects
None yet
Development

No branches or pull requests

3 participants