Add service + allow control over what is autosaved #45

Merged
merged 19 commits into from Dec 15, 2016

Projects

None yet

8 participants

@akonwi
Contributor
akonwi commented Aug 13, 2015

This is a step towards resolving #43. My Git-Plus package observes commit message files and if a save is triggered before the message is finished the commit happens anyway

@akonwi
Contributor
akonwi commented Nov 4, 2015

Has anyone looked at this?

@50Wliu 50Wliu added the needs-review label Nov 4, 2015
@jsg2021
jsg2021 commented Jan 11, 2016

I'm looking forward to this.

@akonwi
Contributor
akonwi commented Dec 8, 2016

@50Wliu Just following up. Does this have a chance at being accepted?

@50Wliu
Member
50Wliu commented Dec 8, 2016

/cc @nathansobo for review

@nathansobo

Thanks! This looks like a cool addition. If you can address my requested changes I think we're close. Perhaps @50Wliu could help with the documentation description if you need input there.

lib/autosave.coffee
@@ -9,6 +10,8 @@ module.exports =
subscriptions: null
+ provideControls: -> dontSaveIf
@nathansobo
nathansobo Dec 13, 2016 Contributor

What do you think about providing an object with a method so we can add other methods without breaking major version compatibility with the existing service?

spec/controls-spec.coffee
@@ -0,0 +1,21 @@
+Controls = require '../lib/controls'
+
+describe 'Controls', ->
@nathansobo
nathansobo Dec 13, 2016 Contributor

I'd also like to see a more integrated test of autosave itself that ensures it interacts with the service and doesn't save if a predicate returns true.

README.md
+
+## Service API
+
+### package.json
@nathansobo
nathansobo Dec 13, 2016 Contributor

This example is great, but could you add some text in english describing this as well?

README.md
+
+### package initialize
+``` coffeescript
+consumeAutosave: (dontSaveIf) ->
@nathansobo
nathansobo Dec 13, 2016 Contributor

Let's use ES6 for this example instead because we're trying to phase out CoffeeScript, though it's going to take a while.

@akonwi
Contributor
akonwi commented Dec 14, 2016

Great! I'll make the changes

@akonwi
Contributor
akonwi commented Dec 14, 2016

I removed the previously written tests for Controls and replaced them with an integrated test using a predicate. The description in the README may need more tweaking though

nathansobo added some commits Dec 14, 2016
@nathansobo nathansobo Rename service method and make version 1.0.0 9d2f80c
@nathansobo nathansobo Make dontSaveIf test more of an integration test a5eb2c3
@nathansobo nathansobo Merge branch 'master' into akonwi-service
1f5cd31
@nathansobo
Contributor

@akonwi I made a few adjustments I'd like to push to your branch... can you follow these instructions to enable me to push to your fork for this branch?

@akonwi
Contributor
akonwi commented Dec 14, 2016
akonwi Update snippet of package.json in README. Remove unused requires in t…
…ests
9f79445
@akonwi
Contributor
akonwi commented Dec 15, 2016

Cool! I just made a couple more changes for cleanup

@nathansobo nathansobo merged commit 9e0470e into atom:master Dec 15, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nathansobo
Contributor

Great additions! Thanks for catching both of those things. Really appreciate you working with us on this one despite the extremely long delay in getting an initial review. Thanks for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment