-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
lib/syncthing: Add wrapper for access to model #9627
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
Conversation
@imsodin I have moved the wrapper over to lib/syncthing as you suggested. I have yet to look into narrowing the scope of this wrapper (i.e. as discussed use FolderSummaryService from the iOS app would remove the need to expose DBSnapshot). My suggestion would be to iterate, i.e. review and merge this now, and I'll have a go at narrowing later. |
Iteration seems fine. Which also means whatever we merge might get released, thus could we please already use the "internal" naming that was brought up before? Basically what you call model or modelwrapper now, lets call that internal.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some naming comments in the review. Few of them I feel a bit stronger about, few of them are just weekly held opinions - the latter begin with "optional:", if you disagree feel free to just e.g. downvote (to ack you saw it) and otherwise ignore, no need to respond why or anything.
Well turned out to be just two comments :)
Otherwise looks good. Would very much like to see moving some of your utility functions into lib/syncthing and reducing the internals surface. Though also the more I look at it, the less bad it looks as is already :)
a9f594a
to
99e1afc
Compare
@imsodin I made some changes, how do you like it now? |
* main: (46 commits) build: use Go 1.23, require minimum 1.22 (syncthing#9651) gui, man, authors: Update docs, translations, and contributors lib/fs: Put the caseFS as the outermost layer (syncthing#9648) gui: Add Irish (ga) translation template (syncthing#9646) gui, man, authors: Update docs, translations, and contributors lib/syncthing: Add wrapper for access to model (syncthing#9627) cli: Remove `go-shlex` dependency (syncthing#9644) lib/sha256: Remove it (syncthing#9643) build: Update dependencies (syncthing#9640) Chmod -x non-executable files (fixes syncthing#9629) (syncthing#9630) gui, man, authors: Update docs, translations, and contributors all: minimal set of changes for iOS app (syncthing#9619) gui, man, authors: Update docs, translations, and contributors gui, man, authors: Update docs, translations, and contributors etc: Remove restart on suspend systemd service (ref syncthing#8448) (syncthing#9611) gui, man, authors: Update docs, translations, and contributors lib/fs: Add missing locks to fakeFile methods (fixes syncthing#9499) (syncthing#9603) lib/api: Increase test request timeout (fixes syncthing#9455) (syncthing#9602) gui, man, authors: Update docs, translations, and contributors lib/ignore: Remove unused patterns in cache (syncthing#9601) ...
Purpose
Wrap access to Model for users that use the syncthing Go package. See discussion: #9619 (review)
Testing
It works with the iOS app. Other than that, there are no current users of this API (to my knowledge) as Model was only exposed recently form the iOS app.
Screenshots
n/a
Documentation
n/a (not user visible)
Authorship
Already in AUTHORS