Remove uri::RsyncModule as a standalone type. #124
Conversation
| /// | ||
| /// The module is identical to the a URI with an empty path. | ||
| pub fn module(&self) -> &str { | ||
| &self.as_str()[..self.path_start] |
There was a problem hiding this comment.
This definition of module() differs from that which was offered by RsyncModule::module() which was defined as being the module name, not the leading part of the Rsync URI upto-and-including the trailing slash after the module name (it was explicitly defined as self.uri.module_name()). This is a change in behaviour while the changelog update gives the impression that the functionality has been moved into the existing Rsync type without change. Is this intentional? Was the RsyncModule::module() fn considered to be wrong and this this is now right? Should the changelog update note this difference in some way?
There was a problem hiding this comment.
Previously, the term was a bit overloaded: module was used as both the URI identifying the module, i.e., the combination of authority and module name – that’s what RsyncModule actually contains, and the name of the module itself within the authority. I decided to clean this up: module now refers the global identifier of the module (i.e., what RsyncModule used to represent) while the identifier of a module within the same authority is now the module name.
This could indeed be clarified in the changelog.
| /// | ||
| /// This is the same as the module but with the authority in canonical | ||
| /// form. | ||
| pub fn canonical_module(&self) -> Cow<str> { |
There was a problem hiding this comment.
Unless I'm misreading, in from_bytes() you permit any case for RsYnC:// yet here you explicitly use rsync:// in lowercase (and then apply make_ascii_lowercase() to it lowering it if it were not already lowered), so in effect you also lowercase the scheme component too.
There is also a very small chance that having two separate occurences of the rsync:// static string in the code could lead to mistakes where one is changed and the other not, perhaps using a single constant in both places would be a minor but worthwhile improvement?
Also, did you consider iterating over the value returned by module() and map uppercase to lowercase on that value rather than construct a new string entirely (potentially not being exactly the same as the type instance currently represents) as you do here?
There was a problem hiding this comment.
The scheme identifier of a URI is case-insensitive (i.e, RsYnC is allowed and equivalent to rsync), so in order to get a canonical URI, I need to lowercase the scheme, too. (In fact RFC 3986 defines the canonical form of the scheme to be lowercase).
I think using the string literal here is fine – why would you want to ever change it?
As for changing the contained value itself, Bytes is a potentially shared type (kind of like an Arc<Box<[u8]>> on steroids) and therefore immutable. So I have to clone the data in order to lowercase it.
uri::Rsynccan be used in place ofRsyncModuleas it is essentially just anuri::Rsyncwith an empty path. I don’t think this invariant is important enough to justify a whole type of its own.