Skip to content

Use uris for paths in git source descriptions #3063

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

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Aug 2, 2021

Sometimes they would be handled as local file-paths leading to broken behviour.

Fixes: #3012

@google-cla google-cla bot added the cla: yes label Aug 2, 2021
@sigurdm sigurdm requested review from jonasfj and natebosch August 2, 2021 14:05
Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

Drive-by comments!

@@ -142,6 +142,8 @@ class GitSource extends Source {
return {'relative': relative, 'url': url};
}

/// Returns [path] normalized.
Copy link
Member

Choose a reason for hiding this comment

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

I'd document it as /// Normalizes [path]. And name it _normalizePath, and say that [path] must be a string or null.

@@ -142,6 +142,8 @@ class GitSource extends Source {
return {'relative': relative, 'url': url};
}

/// Returns [path] normalized.
///
Copy link
Member

Choose a reason for hiding this comment

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

Also, say what a "relative URL" is. It's not clear from context, and not a well-defined term.

The Uri.isAbsolute checks for a scheme and no fragment, so http://example.com/foo#bar is not considered absolute (it has a fragment!)
It's very unlikely to ever be what you want.

You may want uri.hasAbsolutePath instead, and check that url.hasScheme and url.hasAuthority are both false. (Because scheme:relative/path#fragment is not isAbsolute and has a relative path, but is probably not what you'd consider a releative URI reference).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I thought a relative uri was any non-absolute one.

Thanks, will try to see if I can come up with something...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pubspec.lock paths are incompatible on Windows
4 participants