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

Solve issue #36 (Snippets not applied) #66

Merged
merged 2 commits into from
Jun 13, 2015
Merged

Solve issue #36 (Snippets not applied) #66

merged 2 commits into from
Jun 13, 2015

Conversation

almostearthling
Copy link
Contributor

Changes the name of the downloaded gist file to snippets.cson.

F.

@jerone
Copy link
Contributor

jerone commented Jun 12, 2015

Good find! Can you add some specs for this, so it won't happen again.

We also need to be thinking about cleaning up old installs... we probably want to remove the redundant snippets.coffee file.

@almostearthling
Copy link
Contributor Author

Well, I can try to add a test to check that uploaded and downloaded files are the same, as this was the case that would actually break the spec. As a matter of removing snippets.coffee, I don't know how it should be handled: adding code (and load time 😏) just to clean up the mess we've done could be overkill, unless we assume that this will be done by a fixed number of "transitional" releases.

However I don't know how long will it take to me to write the spec, maybe it's worth to fix the thing in order to get it running (a minor release) and add the full-fledged update when there are more features to add.

F.

@almostearthling
Copy link
Contributor Author

I changed my mind: in the specs the test for snippets.cson is missing while the other ones are already there. I'll fix it ASAP.

F.

@jerone
Copy link
Contributor

jerone commented Jun 12, 2015

However I don't know how long will it take to me to write the spec, maybe it's worth to fix the thing in order to get it running (a minor release) and add the full-fledged update when there are more features to add.

Just copy the specs from keymap.cson https://github.com/Hackafe/atom-sync-settings/blob/master/spec/sync-settings-spec.coffee#L84-L91

@almostearthling
Copy link
Contributor Author

That's what I said above in fact, I thought I'd do something more complicated but there's no need to! 😃

F.

@almostearthling
Copy link
Contributor Author

Ok, Travis does not complain with the new spec.

F.

@jerone jerone self-assigned this Jun 13, 2015
jerone added a commit that referenced this pull request Jun 13, 2015
@jerone jerone merged commit 67dbd42 into atom-community:master Jun 13, 2015
@jerone
Copy link
Contributor

jerone commented Jun 13, 2015

👍

@jerone jerone removed their assignment Jun 13, 2015
@jerone jerone mentioned this pull request Jun 13, 2015
@almostearthling almostearthling deleted the solve-snippets branch June 13, 2015 22:03
@jerone
Copy link
Contributor

jerone commented Jun 14, 2015

Ref #36

@jerone jerone added this to the v0.5.0 milestone Jun 23, 2015
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.

2 participants