WiP: URI Rework #1665

charlesdaniels · 2020-12-17T20:18:02Z

Description:

This PR tracks work being done to re-work URIs based on prior discussion (#1233) for the 1.4 release, and also addresses #1509. In particular, it moves several functions into the URI interface that were previously stand-alone functions.

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

Where applicable:

Public APIs match existing style.
Any breaking changes have a deprecation path or have been discussed.

The underlying work to provide implementations of these methods has not been done yet pending discussion, so this will break the build. This also adds Destroy() as described in fyne-io#1509, though again no implementation is defined yet. This also adds detailed comments to the members of the URI interface, to make it clear what is expected of URI implementations.

charlesdaniels · 2020-12-17T20:19:09Z

Would like some input from @andydotxyz / @stuartmscott relating to the proposed changes before I do the legwork to implement. See commends in the diff.

uri.go

storage/uri.go

charlesdaniels · 2020-12-21T18:16:20Z

For the record, I agree with Stuart's changes, but am waiting to put more commits into this until everybody is agreed on the relevant proposal.

This will be immediately reverted, but if we decide to revive it for some reason, it might be a good starting point. I started writing this and then realized Go already has an RFC3986 implementation here: https://golang.org/pkg/net/url/ I plan to use that unless there is some reason to build out or own instead.

This reverts commit adf04ad.

In retrospect, there is a lot of redundancy In the prose of the given implementation. I will do a second pass to tidy it up at some point.

@toaster

Note that I have veered much more towards @toaster 's (Tilo) proposal than I had originally. My change of heart is documented in the relevant proposal here: https://github.com/fyne-io/fyne/wiki/Proposal%3A-The-Fyne-URI-Philosophy I have decided not to implement URINotListable on the basis that URIOperationImpossible already encompasses it's purpose, and additionally I want to encourage people to use Listable() rather than just trying to List() and then checking the error. I decided to move List and Listable() into the Repository interface, though developers can always throw URIOperatioNotSupported if they are impossible to implement. I could be convinced to move them into their own ListableRepository interface to be optionally registered, but to keep them together (since you cannot have one without the other). This was discussed a little bit, but I am on the fence. Since I decided to go with more of Tilo's approach, I did not add Move() or Duplicate(). If folks have opinions on using these names versus Rename() and Copy(), please add them on the PR.

charlesdaniels · 2021-01-08T03:33:55Z

I have sketched out more or less what my current thinking is on the API in a87a205 and also wrote a commit message with some more thoughts.

If people have objections, now is the time to raise them. We are getting down to the wire on 2.X. I have done my very best to balance what I believe will be a good API design, what will be backwards compatible with the existing 1.4.X API, and what we have discussed in previous meetings and in the proposal.

One thing I would like explicit comments on is whether folks feel we should move Repository into it's own package, or keep it in storage. I am on the fence, but leaning towards moving it into storage/repository, to reduce the clutter in the storage package - most developers will not need to interact with repositories directly, so hiding it makes sense.

charlesdaniels · 2021-01-08T03:35:38Z

Specifically paging @andydotxyz, @toaster, @stuartmscott, and @AlbinoGeek w.r.t the above comment.

AlbinoGeek

While I'm currently having a hard time (mentally) putting together an implementation for the Repository, after reviewing the code I feel it should be straight-forward to use. However, an example would go a long way in helping that.

I may have missed this somewhere, should file:// scheme be implemented as a Repository, to make sure it conforms with the interface, etc? (this would also be a great way to tell if any major / important features were missing from the API.)

That would serve as a great example. Does it belong in another PR or this one?

Overall I like the direction and can't wait to see what others think too.

AlbinoGeek · 2021-01-08T03:53:06Z

storage/repository.go

+// the appropriate repository.
+//
+// NOTE: functions in a particular repository can assume they will only ever be
+// used on URIs which match the scheme that they have been registered to


This wording (to me, when I read it) implied an almost one-to-one relationship of Repository to scheme, where a Repository may be capable of handling multiple schemes. Cases would even exist where multiple repositories support the same scheme.

AlbinoGeek · 2021-01-08T03:56:30Z

storage/uri_operation_impossible.go

+// is supported by the underlying implementation, but which violates some
+// internal constraints of that particular repository implementation. For
+// example, creating an un-representable state might cause this error to occur.
+const URIOperationImpossible uriOperationImpossible = uriOperationImpossible("The requested URI operation is not possible.")


Should this be worded in a way where it more specifically refers to the operation not being possible with that URI, or is this error also used in contexts outsides of an error specific to one repository implementation?

andydotxyz

This looks really solid.
A few changes requested and thoughts inline but I think this fulfils the requirements.

Some other thoughts back on the URI philosophy document so it's consistent.

andydotxyz · 2021-01-08T14:48:24Z

storage/repository.go

+	// to a particular URI scheme.
+	//
+	// Since 2.0.0
+	Init() func() error


I'm not sure about including Init to be run during register - if we are supporting a single Repository backing multiple schemes then this could be called multiple times, which the name does not imply.
What is this needed for that cannot be expected of the implementation before calling Register?

I am also not sure this needs to be included. I envisioned the initialization going the other way, where the developer initializes a repository, most likely by importing a third party library, which runs its func init(), which registers itself with the storage API (like when importing "image/png")

andydotxyz · 2021-01-08T14:49:07Z

storage/repository.go

+// error if it does.
+//
+// Since 2.0.0
+func RegisterRepository(scheme string, repository Repository) error {


Given that Repository is the "main" interface, I would suggest Register(string, Repository) so it is more clearly differentiated from the optional registrations above.

andydotxyz · 2021-01-08T14:51:46Z

storage/uri.go

+// with a URIOperationNotSupported error.
+//
+// Since 2.0.0
+func Listable(u fyne.URI) (bool, error) {


Listable feels almost an assertion, would CanList be OK? It feels less ambiguous.

Or IsListable

That crossed my mind to, though generally we prefer to drop "Get" and "Is" prefixes.

andydotxyz · 2021-01-08T14:56:05Z

storage/uri_operation_impossible.go

+package storage
+
+// Declare conformance with Error interface
+var _ error = URIOperationImpossible


I'm sure it would be OK to have all declared errors in a single error.go file :)

I agree, I also don't think the separate types is very go idiomatic. https://golang.org/src/io/io.go has some exported errors that I have used before;

var EOF = errors.New("EOF")

Discussing idomatic errors in Go is tricky as they are mid-transition ;) We need to go with what was best in 1.12 I think as some of the newer things we can't opt in to without upgrading our version required.
That said I don't remember what best practice was 3 versions ago.

andydotxyz · 2021-01-08T14:57:24Z

uri.go

+	// Since: 2.0.0
+	Authority() string
+
+	// Path should return the URI path, as defined by IETF RFC3986.


We need to add to this doc to make it clear that one must not assume a file path.
As we use URI to replace file handling I have heard many assume that "IF you would just implement Path() then I could send it to my file managent code...

stuartmscott · 2021-01-08T16:21:36Z

storage/repository.go

+	// to a particular URI scheme.
+	//
+	// Since 2.0.0
+	Init() func() error


I am also not sure this needs to be included. I envisioned the initialization going the other way, where the developer initializes a repository, most likely by importing a third party library, which runs its func init(), which registers itself with the storage API (like when importing "image/png")

stuartmscott · 2021-01-08T16:23:02Z

storage/repository.go

+// repository.
+//
+// Since 2.0.0
+func RegisterCopy(scheme string, copyImplementation func(fyne.URI, fyne.URI) error) error {


nit:

Suggested change

func RegisterCopy(scheme string, copyImplementation func(fyne.URI, fyne.URI) error) error {

func RegisterCopy(scheme string, copy func(fyne.URI, fyne.URI) error) error {

stuartmscott · 2021-01-08T16:24:13Z

storage/uri.go

+// with a URIOperationNotSupported error.
+//
+// Since 2.0.0
+func Listable(u fyne.URI) (bool, error) {


Or IsListable

stuartmscott · 2021-01-08T16:27:46Z

storage/uri_operation_impossible.go

+package storage
+
+// Declare conformance with Error interface
+var _ error = URIOperationImpossible


I agree, I also don't think the separate types is very go idiomatic. https://golang.org/src/io/io.go has some exported errors that I have used before;

var EOF = errors.New("EOF")

storage/repository.go

charlesdaniels · 2021-01-08T21:39:01Z

@andydotxyz @stuartmscott @AlbinoGeek

Per our meeting today, I have written a revised proposal (link). I think this represents our current consensus based on what we discussed.

If anyone has final comments on the URI proposal, Monday the 11th is the deadline to speak or forever hold your peace. Please use the new discussion section I added for the updated proposal. I will keep editing it in-place to track what we want the final API to be.
We went over the comments on this PR that are currently open in our meeting, and I think the revised proposal addresses them. If so, please mark the conversations as resolved.

I plan to resume work on this on Monday (or maybe a little on Sunday).

charlesdaniels added 2 commits December 17, 2020 15:14

add Since: comments

e904cf8

charlesdaniels changed the base branch from master to develop December 17, 2020 20:18

charlesdaniels mentioned this pull request Dec 17, 2020

Delete/Rename URIs #1509

Closed

stuartmscott reviewed Dec 19, 2020

View reviewed changes

uri.go Outdated Show resolved Hide resolved

uri.go Outdated Show resolved Hide resolved

uri.go Outdated Show resolved Hide resolved

uri.go Outdated Show resolved Hide resolved

storage/uri.go Outdated Show resolved Hide resolved

charlesdaniels mentioned this pull request Dec 29, 2020

Mobile simulation missing file / listable support #1470

Closed

charlesdaniels added 8 commits January 4, 2021 12:06

Merge branch 'develop' into uri_rework

e998ab1

Revert "add storage/rfc3986"

1017633

This reverts commit adf04ad.

first pass at Repository interface

c363bbc

In retrospect, there is a lot of redundancy In the prose of the given implementation. I will do a second pass to tidy it up at some point.

add stuarts suggestions

c764b37

remove functions from URI interface

d6608cb

add RFC3986 methods to the URI interface

a68a6b2

AlbinoGeek reviewed Jan 8, 2021

View reviewed changes

andydotxyz requested changes Jan 8, 2021

View reviewed changes

stuartmscott reviewed Jan 8, 2021

View reviewed changes

storage/repository.go Show resolved Hide resolved

charlesdaniels changed the base branch from develop to storage_repository January 8, 2021 21:41

charlesdaniels marked this pull request as ready for review January 8, 2021 21:41

charlesdaniels merged commit bbf9299 into fyne-io:storage_repository Jan 8, 2021

charlesdaniels mentioned this pull request Jan 11, 2021

Storage repository #1768

Merged

This was referenced Jan 14, 2021

Deprecate fyne.ListableURI #1802

Open

Deprecate ListableURI #1803

Closed

charlesdaniels mentioned this pull request May 10, 2021

Add time infos in the storage package #2233

Open

charlesdaniels added the Storage changes relating to Storage Repositories and URIs label May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WiP: URI Rework #1665

WiP: URI Rework #1665

charlesdaniels commented Dec 17, 2020

charlesdaniels commented Dec 17, 2020

charlesdaniels commented Dec 21, 2020

charlesdaniels commented Jan 8, 2021

charlesdaniels commented Jan 8, 2021

AlbinoGeek left a comment

AlbinoGeek Jan 8, 2021

AlbinoGeek Jan 8, 2021 •

edited

andydotxyz left a comment

andydotxyz Jan 8, 2021

stuartmscott Jan 8, 2021

andydotxyz Jan 8, 2021

andydotxyz Jan 8, 2021

stuartmscott Jan 8, 2021

andydotxyz Jan 8, 2021

andydotxyz Jan 8, 2021

stuartmscott Jan 8, 2021

andydotxyz Jan 8, 2021

andydotxyz Jan 8, 2021

stuartmscott Jan 8, 2021

stuartmscott Jan 8, 2021

stuartmscott Jan 8, 2021

stuartmscott Jan 8, 2021

charlesdaniels commented Jan 8, 2021

	func RegisterCopy(scheme string, copyImplementation func(fyne.URI, fyne.URI) error) error {
	func RegisterCopy(scheme string, copy func(fyne.URI, fyne.URI) error) error {

WiP: URI Rework #1665

WiP: URI Rework #1665

Conversation

charlesdaniels commented Dec 17, 2020

Description:

Checklist:

Where applicable:

charlesdaniels commented Dec 17, 2020

charlesdaniels commented Dec 21, 2020

charlesdaniels commented Jan 8, 2021

charlesdaniels commented Jan 8, 2021

AlbinoGeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbinoGeek Jan 8, 2021 • edited

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesdaniels commented Jan 8, 2021

AlbinoGeek Jan 8, 2021 •

edited