-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Support [F-Droid] custom repos #10792
Open
cyb3rko
wants to merge
4
commits into
badges:master
Choose a base branch
from
cyb3rko:fdroid-custom-repo
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
shouldn't we merge these two into one input? just endpoint, there is no need to split here.
Also i think
Joi.string()
is too loose for part of a url and could intrudoce invalid input that passes this test.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.
I would rather keep it separate, as they are different components in the URL.
If we would combine them, how would you call it to not confuse the user?
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.
You're right about the endpoint format. What about "relativeUri" from validators.js?
It seems other services use the same validator for paths.
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.
In other services we use something like
gitea_url
orgitlab_url
i propose using something similar.
notice that in some tests the base url can even include get params.
also you can use the
optionalUrl
for the schema.we could keep the project consistency and use
fdroid_url
herei recommend you to take a look at one of the services already using hosted instances like gitea
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.
Passing the full URL does not feel right.
Especially with #2637 in mind, at least the protocol should be hard-coded to https.
The way Matrix (https://shields.io/badges/matrix) badges do it seems much better and "cleaner" for the user.
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.
Thank you for highlighting #2637. It seems like a valuable addition that could be implemented as a project-wide feature. While you can explore this idea in the current PR, I believe it would be better to separate these two subjects and ensure each PR focuses on a single topic for clarity and maintainability.
As a temporary solution, you could consider introducing a new validator,
optionalUrlHttps
, invalidators.js
. This would address the immediate need without over complicating this PR.Regarding the Matrix badges, it uses only the
FQDN
for thehost/endpoint
. The mode parameter, however, serves a distinct purpose—it separates result types and facilitates different goals beyond accessing the endpoint. The key takeaway here is that mode provides functionality tied to achieving separate objectives.Could you provide an example where your proposed
endpoint
achieves a distinct goal separate from simply accessing the same information? For comparison, consider how the Matrix badge's separation ofFQDN
and mode addresses two clearly different goals.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.
A URL per RFC1738 also contains the scheme so we have to find a different name.
I see what you mean, so let's consider using only one parameter instead.