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.

Sign up for GitHub

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

Jump to bottom

Fix #754 and #865 #872

Merged
merged 11 commits into from
Jun 24, 2019
Merged

Fix #754 and #865 #872

merged 11 commits into from
Jun 24, 2019

Conversation

protiumx
Copy link
Contributor

@protiumx protiumx commented May 3, 2019

Short description of what this resolves:

Fix #754 and #865

Only my commits as requested

protiumx added 5 commits May 3, 2019 19:09
@protiumx
Fix #754
95abd9c 
The pragma util keeps a copy of the settings that are upload to the
gist. The file is located at <context.globalStoragePath>/settings.sync.

If the content to be upload doesn't deffer from the local content after
taking out the @sync-ignore, the file wont be uploaded.
@protiumx
Fix #754: Sync object
0707b1e 
Checks the global storage path at constructor.
Checks if the content of the setting file should be uploaded.
@protiumx
Update tests
fafa97e
@protiumx
Fix #865
4e662ad
@protiumx
Fix broken tests
2b49404
@shanalikhan
Copy link
Owner

can you solve the conflicts so i can merge it ?

@shanalikhan shanalikhan added this to the v3.3.0 milestone May 6, 2019
@shanalikhan
Copy link
Owner

Same fix merged for the PR #877.
You might need to test this after rebasing and solving the conflicts
https://github.com/shanalikhan/code-settings-sync/pull/877/files#diff-a3281403b8563f0dc6c841b4f197c14dR242

@protiumx
Copy link
Contributor Author

protiumx commented May 6, 2019

@shanalikhan have you added pragma support to other files? If yes I should add code for those files too.
Changes from #877 on file pragmaUtil.ts can be taken. For the sync.ts I suggest to leave my changes until I drop another PR for the other setting files (FILE_KEYBINDING_MAC, FILE_KEYBINDING_DEFAULT). Let me know what do you think.

@shanalikhan
Copy link
Owner

have you added pragma support to other files? If yes I should add code for those files too.

Yes, PR #854 has been merged in v3.3.0 branch that includes the support for pragmas for keybindings.json as well.

You can test it on your machine and improve the scenario for it as well if you think we can improve.

@fgallardograzio
Copy link

fgallardograzio commented May 13, 2019 •
edited
Loading

@ioprotium Is it possible that the changes you introduced related to #754 are causing the extension to remove the settings.json file from the gist when the user manually runs the "Sync: Update / Upload Settings" command without having changed said file?

@protiumx
Copy link
Contributor Author

@shanalikhan then I will add code for those files. We need to define integration test for the whole process, from the file changes detection to the upload. Do you have any ideas to accomplish it?

@fgallardograzio It may causes to not upload the file in some scenario. Please wait to may next PR I will check this.

@shanalikhan
Copy link
Owner

We need to define integration test for the whole process, from the file changes detection to the upload. Do you have any ideas to accomplish it?

Settings Sync use chokidar in the backend to detect the file change, what we can do it to see if they provide options to see the trail of file contents that are changed.

@shanalikhan shanalikhan changed the title Fix #754 and #865 [WIP] : Fix #754 and #865 May 17, 2019
@protiumx
Copy link
Contributor Author

@shanalikhan there is still work to do in order to add support to the other files.
Wait for my next PR

@shanalikhan
Copy link
Owner

shanalikhan commented May 17, 2019 •
edited
Loading

ok let me know once it is ready to merge for the fixes #754 and #865. I will merge it

@shanalikhan shanalikhan modified the milestones: v3.3.0, Backlog Jun 6, 2019
@shanalikhan
Copy link
Owner

@ioprotium any idea when you can fix this two issues?

@protiumx
Copy link
Contributor Author

@shanalikhan I'll try to fix them tomorrow. But basically, if you added support for pragmas in every file, we need a local copy of each file to check when they actually change. Besides that I think there is an issue: if the file is not uploaded is deleted from the gist?

@auxves
Copy link
Contributor

auxves commented Jun 17, 2019

@ioprotium

if the file is not uploaded is deleted from the gist?

Yes, I have experimented with this before and can confirm this.

@protiumx
Copy link
Contributor Author

@arnohovhannisyan @shanalikhan well, then we should verify for each file. If one or more have changed, we need to upload every file.

@protiumx
Copy link
Contributor Author

@shanalikhan I've solved the file conflict but project will still be broken.
This weekend I'll add more commits to this PR or create a new one. I'll let you know

@protiumx
Fixing Octokit wrong type for GistsResponse
2158ed3 
'file' field is wrong with static data.
protiumx added 3 commits June 22, 2019 19:21
@protiumx
Defining PragmaUtil behavior
1f729b9 
Pragma util should only parse string content, checking each line for pragma statements.
@protiumx
Updating test for Pragma definition
0d5129a
@protiumx
Sync: don't upload is not necessary
356e17a 
As the extension gets the entire gists before commiting an upload, we can check each file of the gists against our local files in order to determine if the content has changed.

There is no need to keel a local copy.
@protiumx
Copy link
Contributor Author

protiumx commented Jun 22, 2019 •
edited
Loading

@shanalikhan my last commits fix the problem. I've noticed that the extension was already getting the gist before uploading the files. So I compare against the remote files. The entire gist will be updated if one or more local files have changed.

Note:

  • I couldn't get yarn run test working. I get the "Can't find vscode module". Launch test is also broken
  • I added a fix for typing from octokit.

To have in mind:

  • Sync.ts is getting big, unreadable and in consequence unmaintainable

This PR is ready to be merge.

@auxves
Copy link
Contributor

auxves commented Jun 23, 2019

@ioprotium

Sync.ts is getting big, unreadable and in consequence unmaintainable

I was actually thinking of refactoring sync.ts + some other long files into separate files after v3.3.0 gets released.

@protiumx
Copy link
Contributor Author

@arnohovhannisyan cool!

@shanalikhan shanalikhan modified the milestones: Backlog, v3.3.0 Jun 24, 2019
@shanalikhan shanalikhan changed the title [WIP] : Fix #754 and #865 Fix #754 and #865 Jun 24, 2019
@shanalikhan shanalikhan merged commit c0ee128 into shanalikhan:v3.3.0 Jun 24, 2019
@protiumx
Copy link
Contributor Author

@shanalikhan I'd try to become a more active collaborator, I think we can improve it taking into account all user's suggestion. This pragma util turns out to be an alternative to github versioning of the settings, since you only have to specify which settings should be applied to your dev environment.

I'll take a loot at the open issues when I find time.

@protiumx protiumx deleted the PR/754 branch June 24, 2019 15:05
@shanalikhan
Copy link
Owner

@ioprotium
Thanks for the fix. The Pragma Util has become the best choice for the users dealing with the Github GIST since it covered alot of scenarios.

Feel free to improve. I might release new version this week.
Also if you think we can improve the public wiki for Pragma Util, you can improve that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug 🐛 improvement ✨
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@protiumx @shanalikhan @fgallardograzio @auxves