Open theon opened 8 years ago
0.00%
Merging #113 into master will decrease coverage by -91.94% as of
50add00
@@ master #113 diff @@
=====================================
Files 21 21
Stmts 236 243 +7
Branches 14 9 -5
Methods 0 0
=====================================
- Hit 217 0 -217
Partial 0 0
- Missed 19 243 +224
Review entire Coverage Diff as of
50add00
Powered by Codecov. Updated on successful CI builds.
An alternate solution, inspired by RFC 3986 3. Syntax Components and 3.3. Path, could be:
sealed trait Path(pathParts: Seq[PathPart], hasRootSlash: Boolean) { ... }
case class AbsolutePath(pathParts: Seq[PathPart]) extends Path(pathParts, true) { ... }
This would be used by: full and absolute http[s], etc.
class RootlessPath(pathParts: Seq[PathPart]) extends Path(pathParts, false) { ... }
This would be used by: relative http[s], mailto, urn, etc.
object EmptyPath extends RootlessPath(Seq.empty) { ... }
In Uri, pathParts: Seq[PathPart]
could be replaced with path: Path = EmptyPath
.
If a Uri has an authority, then the path must be AbsolutePath or EmptyPath.
Thanks @evanbennett. I like it. We could maybe do the same to the Uri
class and split that into several classes so that the type system enforces your last point.
trait Uri[P <: Path] {
def path: P
// Generic methods that use Path
}
case class AbsoluteUri(scheme: String, host: String, path: AbsolutePath) extends Uri[AbsolutePath]
// ProtocolRelativeUrl does not have a scheme
case class ProtocolRelativeUri(host: String, path: AbsolutePath) extends Uri[AbsolutePath]
case class DocumentRelativeUri(path: RootlessPath) extends Uri[RootlessPath]
case class SiteRelativeUri(path: AbsolutePath) extends Uri[AbsolutePath]
At first I thought that SiteRelativeUri
having an AbsolutePath
was a bit odd, but I think it makes sense if it is fair to say the definition of an AbsolutePath
is just a path starting with a slash. I think I prefer the name RelativePath
over RootlessPath
as it mirrors with the AbsolutePath
name.
Being such a big change, this would be a major version bump. I haven't been using semantic versioning properly, so maybe this would be a good chance to start with version 1.0.0
I have been researching and thinking about this since I posted. One of the things that I wanted to do was to have multiple Uris as you have suggested. (I was not sure that you would have been interested in such a big change.)
One of the other things that I noted was that when parsing, you have:
case class Authority(user: Option[String], password: Option[String], host: String, port: Option[Int])
case class UserInfo(user: String, pass: Option[String])
but you do not use them in the Uri. If we were going to subclass the Uri, would you be open to moving these class to the uri
package and using them within the Uri class\trait?
Would you like me to put something together in my fork?
I'd absolutely be interested. Even though it would be a big change, I think it would be a good move.
I'm happy for Authority
to be used in the Uri
class as long as it is still convenient to get the host + port etc from a Uri
instance (myUri.host
rather than myUri.authority.host
)
I have pushed to my repository the updates I have been working on.
Path
classesUri
type sub-classesAuthority
and UserInfo
classesParameter
class as mentioned in #34No DSL changes (aside from compatibility changes), this will be completed as a separate task.
I started updating the README, but as I am unsure what will get accepted, I stopped.
I have not submitted a pull request, as I am not sure you will like all the changes. You may just want pieces of the changes, I am not sure. I just went ahead and implemented what I am happy with.
If you were to want to move in this direction, I was thinking we could release v1.0.0 which has only a few backwards incompatibilities and a lot of deprecated methods to ease migration. Once, v1.0.0 is released, we could remove all the deprecations, and that would then be released as v1.1.0 straight away.
Here is a list of the changes (that I am aware of) that are required to migrate an existing project to use v1.0.0:
Uri
method changes:
scheme
use protocol
insteadquery
use queryValue
insteadfragment
use fragmentString
insteadcopy
use copyOld
insteadpath
use pathToString
insteadunapply
returns the new arguments (I was unable to work out how to implement the old unapply
method and deprecate it. I just though that I could create another object (maybe UriOld
) with the old unapply
method?)Incompatibilities:
toString
s as "?", and Uri
can have no query.There quite a few TODO
s throughout the code. Aside from DslTests
which I have not worked on yet, and Trie
which previously existed, they are things that I wanted to bring to your attention. They include the incompatibilities list above.
I am going to work on updating the DSL while I await your feedback.
Thanks for the work. Amazing contribution! I'll make a 1.x
branch in this repo and open a pull request to start. We can talk about specifics inline on that PR.
Once we get to a place we are really happy I'll branch the current master into a 0.4.x
branch and the 1.x
can be merged into master. We can make some 1.x
releases before that point though.
Because it is such a big change it might take a little while for me to review, but I'll get on it ASAP.
I have completed my first attempt at the new DSL that is consistent with the new types and follows the conversation in #117.
While implementing the tests, I felt the need to default parsing to RFC3986 compliant and enable delimiter parsing through UriConfig
. This seems more consistent with the README as well.
I have not updated the README yet (same reason as before), and some documentation probably needs to be updated/added, but otherwise, I think this is complete and ready for review.
Have you made any progress on your previous comment? (I am not trying to harass you. I am not very experienced with git and GitHub, and thought I might have missed something.)
I was wondering if you had a chance to look into this?
This PR looks like it resolves https://github.com/NET-A-PORTER/scala-uri/issues/107. Is there any status update on this?
Hi @Petesta, sorry for the lack of updates on this. I am no longer part of the Net-A-Porter organisation and unfortunately there is unlikely to be any updates to this repo in the short term. I would like to fork a new version of scala-uri at some point based on @evanbennett's PR, but can't make any promises on when.
Possible fix for #107.
This change uses a leading empty path part for situations where we want a leading slash in the path. I'm not overly convinced this is the best way to fix this. Open to other suggestions.