Skip to content

feat(settings): custom shortcuts #1941

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noirbizarre
Copy link
Contributor

@noirbizarre noirbizarre commented Jan 20, 2025

It makes the prefixes (currently gh: and gl:) customizable using user settings, allowing to:

  • add your own (remote and local)
  • override the existing ones (especially to force ssh access on private repositories)
  • benefit from specific GitHub and GitLab behavior on alternative, private or corporate instances

It moves URL normalization into the settings to benefit from custom shortcuts.

Some decisions are required on the vcs.get_repo() behavior:
it currently only assumes https://gitlab and https://github are http git repositories, unless prefixed with git+.
However, non git remotes are not supported, so I wonder if it would be simpler to consider all https:// as git repositories (it would fail if this is not the case, whatever this method returns)

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.93%. Comparing base (1dabe8e) to head (2881060).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1941      +/-   ##
==========================================
- Coverage   97.93%   97.93%   -0.01%     
==========================================
  Files          53       53              
  Lines        5677     5705      +28     
==========================================
+ Hits         5560     5587      +27     
- Misses        117      118       +1     
Flag Coverage Δ
unittests 97.93% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Quick initial review 🙂

@noirbizarre noirbizarre marked this pull request as ready for review March 2, 2025 16:58
@noirbizarre
Copy link
Contributor Author

So, back to this one !
As written in the PR description 👆🏼, given this discussion, I'd like to know the direction I should give to this PR:

  • should it assume that all http URLs are git repositories, given this is the only supported case? (This what has been done in the test of this PR)
  • about URL normalization, which this PR has mostly move to settings, what should be done ?

This will mostly have impact on get_repo() and GIT_PREFIX

I will adjust this PR accordingly, but I need some decision and directions.

@sisp
Copy link
Member

sisp commented Mar 3, 2025

  • should it assume that all http URLs are git repositories, given this is the only supported case? (This what has been done in the test of this PR)
  • about URL normalization, which this PR has mostly move to settings, what should be done ?

IMO, it would be consistent to treat URLs identically when passed as a source to copier {copy,recopy} and when listed as trusted URLs in user settings. If you use the same logic as the Template.url_expanded property, then all HTTP and SSH URLs are treated as Git URLs, and if they can't be parsed using get_repo(), then the original URL is used as a fallback for, e.g., local paths. Off the top of my head, I can't think of a security problem when also normalizing the trusted URLs like this. And not normalizing trusted URLs, i.e. requiring a user to provide a normalized URL in the list of trusted URLs, feels brittle and unexpected by users.

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

Successfully merging this pull request may close these issues.

3 participants