Update the sync command #211
Conversation
Previously, setting up a list to sync with ultralist.io was fairly convoluted. Bot the `init` and `sync` commands were dependent on a list's sync state, and the behavior of these commands would change based upon that. **Previous flow:** * `ultralist init` - Create a new list and optionally, sync it with Ultralist.io. * `ultralist sync` - Depending if a list is synced or not, it does multiple things: * If not synced, sets up the local list to sync to ultralist.io. * If is already synced, pull remote changes to local, and push any local changes to remote. This PR simplifies the commands by adding a couple of flags to the `sync` command. The result is that each command has one job to do, instead of many dependent on state. It's simpler to understand as well. **With this PR:** * `ultralist init` - makes this command do just one thing - initialize a list. It does not handle syncing a list as well. * `ultralist sync --setup` - sets up a local list to sync with ultralist.io, or pulls a list from ultralist.io and replaces what's local. * `ultralist sync --unsync` - stops a local list from syncing with ultralist.io. * `ultralist sync` - just handles the actual list syncing between local and ultralist.io.
| } | ||
| } | ||
| // pull a list from ultralist.io | ||
| type Response struct { |
There was a problem hiding this comment.
may be its me not being a full time golang programmer, but a struct, definition inside of a func seems a little odd (unless you are trying to hide it)
| func (a *App) SetupSync() { | ||
| backend := NewBackend() | ||
| if !backend.CredsFileExists() { | ||
| fmt.Println("You're not authenticated with ultralist.io yet. Please run `ultralist auth` first.") |
There was a problem hiding this comment.
this error isnt strictly true, it is only checking for if the file exists and could contain bad auth keys.
NewBackend(), already check and loads this file, so would a more correct test be.
if backend.Credentials == "" {
fmt.Println("You're not authenticated with ultralist.io yet. Please run `ultralist auth` first.")
return
}
Side note: If you are using JWT (I dont have a UL account so I am guessing) as the authentication, should we also check to see it is valid before trying to use it? or as part of the loadCreds, or a new function checkCreds.
There was a problem hiding this comment.
ah, that's a good catch!
Regarding the JWT, I don't feel strongly about validating, since the request would ultimately fail if they have invalid creds.
There was a problem hiding this comment.
I did try it with just using backend.Creds, and it didn't seem to work. idk 🤷
There was a problem hiding this comment.
try with a ~/.config/ultralist/cred.json with {}.
Also a blank file will cause a panic when NewBackend() tries to parse it.
I was thinking about turning those panics into a log and non-0 exit.
There was a problem hiding this comment.
I'm not sure there's a scenario where creds.json would have that info in it though.
also, refactor file_store so it is less rigid.
| // FileStore is the main struct of this file. | ||
| type FileStore struct { | ||
| FileLocation string |
There was a problem hiding this comment.
removing this breaks tests.
# github.com/ultralist/ultralist/ultralist [github.com/ultralist/ultralist/ultralist.test]
./file_store_test.go:13:22: unknown field 'FileLocation' in struct literal of type FileStore
./file_store_test.go:18:23: unknown field 'FileLocation' in struct literal of type FileStore
FAIL github.com/ultralist/ultralist/ultralist [build failed]
ultralist/file_store.go
Outdated
| // Returns if a local .todos.json file exists in the current dir. | ||
| func (f *FileStore) LocalTodosFileExists() bool { | ||
| dir, _ := os.Getwd() | ||
| localrepo := fmt.Sprintf("%s/.todos.json", dir) |
There was a problem hiding this comment.
would filepath.Join(dir, FILENAME) be more robust?
|
thanks for your review, @stuartskelton ! |
Previously, setting up a list to sync with ultralist.io was fairly convoluted. Bot the
initandsynccommands were dependent on a list's sync state, and the behavior of these commands would change based upon that.Previous flow:
ultralist init- Create a new list and optionally, sync it with Ultralist.io.ultralist sync- Depending if a list is synced or not, it does multiple things:This PR simplifies the commands by adding a couple of flags to the
synccommand. The result is that each command has one job to do, instead of many dependent on state. It's simpler to understand as well.With this PR:
ultralist init- makes this command do just one thing - initialize a list. It does not handle syncing a list as well.ultralist sync --setup- sets up a local list to sync with ultralist.io, or pulls a list from ultralist.io and replaces what's local.ultralist sync --unsync- stops a local list from syncing with ultralist.io.ultralist sync- just handles the actual list syncing between local and ultralist.io.