-
Notifications
You must be signed in to change notification settings - Fork 52
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
Copy Python tests for Modulemd.ModuleStream.tracker into C #410
Conversation
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.
Overall it LGTM!
|
||
g_clear_pointer (&tracker_prop, g_free); | ||
|
||
// Test Unicode values |
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.
According to the docs, the tracker
is:
The website address of the upstream bug tracker for this module.
So it doesn't really make sense to have Unicode characters in a website URL. Now we should either remove this test assuming no one is ever going to add a URL containing Unicode or handle this case in the main function by returning NULL
whenever we encounter an invalid URL.
@sgallagher What do you suggest?
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.
Leave it as-is for now. There's nothing stopping someone right now from putting unicode into the metadata here. We should probably look at using http-parser or llhttp to validate that we have validly-formatted URLs though. Mind opening a ticket to track that?
|
||
g_clear_pointer (&tracker_prop, g_free); | ||
|
||
// Test Unicode values |
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.
Same as above.
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.
This patch has conflicts with the master branch. Please run git rebase
and fix them up.
|
||
g_clear_pointer (&tracker_prop, g_free); | ||
|
||
// Test Unicode values |
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.
Leave it as-is for now. There's nothing stopping someone right now from putting unicode into the metadata here. We should probably look at using http-parser or llhttp to validate that we have validly-formatted URLs though. Mind opening a ticket to track that?
@sgallagher I've rebased this branch onto master. |
This is related to #199. I also added Unicode tests.