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

Data files #318

Merged
merged 10 commits into from
Apr 20, 2020
Merged

Data files #318

merged 10 commits into from
Apr 20, 2020

Conversation

rigrig
Copy link
Contributor

@rigrig rigrig commented Apr 17, 2020

Make these apps use data files:

  • widbatpc
  • welcome
  • setting
  • alarm
  • heart
  • ncstart
  • numerals
  • gpsrec

gpsrec uses .gpsrcN to store tracks, so sanitycheck still complains about the filename not containing the app Id :-(

@@ -1,13 +1,13 @@
// The welcome app is special, and gets to use global settings
(function(back) {
let settings = require('Storage').readJSON('welcome.settings.json', 1)
let settings = require('Storage').readJSON('welcome.sjson', 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, well spotted.

@gfwilliams
Copy link
Member

Looks great - am I missing something about the settings though? Maybe it's fine but I'd be a little concerned about removing the default settings file in case some other app actually expects that there'll be values in it...

settings: never delete settings file
welcome: fix typo, remove outdated comment
ncstart: remove outdated comment
@rigrig
Copy link
Contributor Author

rigrig commented Apr 20, 2020

I looked at all apps, and as far as I could tell they all handled missing setting.json properly.
The problem with including a default settings file is that it gets overwritten every time you update the app. Never deleting the global settings file seems like a good idea though.

# Conflicts:
#	apps/setting/ChangeLog
#	apps/widbatpc/ChangeLog
@gfwilliams
Copy link
Member

Thanks! Definitely removing the default file seems like a good plan - there's so much duplication at the moment

@gfwilliams gfwilliams merged commit 14df398 into espruino:master Apr 20, 2020
@gfwilliams
Copy link
Member

Just to add, one other option for settings is a overwrite:true option that makes the IDE insert the code: if (!storage.list(filename)) storage.write(filename, ...)

@rigrig
Copy link
Contributor Author

rigrig commented Apr 20, 2020

We could do something like that, but do we actually want to upload default setting files?

I think apps should be able to handle missing settings. If only because a new app version might add a setting, so the updated app needs to handle the old file where that setting is missing.
The logical way to handle missing setting is to use the default value, so there is no point in uploading a default settings file: those values are coded in anyway so you would only duplicate them some more.

(Maybe apps could include a settings-default file which is never actually used, as documentation?)

@gfwilliams
Copy link
Member

Yes, good point :) Thanks!

@rigrig rigrig deleted the data_files branch April 29, 2020 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants