sshaw / git-link

Emacs package to get the GitHub/Bitbucket/GitLab/... URL for a buffer location
399 stars 74 forks source link

Add plain=1 to force showing a non rendered page in Github links #98

Closed erickgnavar closed 2 years ago

erickgnavar commented 2 years ago

GitHub and other services shown a rendered version for some file types by default, we defined a customizable list of extensions so we can avoid this behaviour using a querystring flag, in the case of GitHub

sshaw commented 2 years ago

Hi, thanks for the PR. This always annoyed me so glad it's mostly fixed. I say mostly because we generate links without a line number. This happens when one:

Can you update it to always include ?plain=1?

I do wonder if there's an edge case where plain=1 will cause undesirable behavior?

erickgnavar commented 2 years ago

Hi, thanks for the PR. This always annoyed me so glad it's mostly fixed. I say mostly because we generate links without a line number. This happens when one:

  • Specifies - as a prefix arg
  • Calls git-link when in a dired buffer, magit buffer, or similar

Can you update it to always include ?plain=1?

I do wonder if there's an edge case where plain=1 will cause undesirable behavior?

done in cc430351b3314c400c1f813ad5b07e050db113ba, btw do you think this should be enable using a configuration variable? So default behaviour(which includes the "bug") won't be affected when someone update the package and they can activate it later by customizing that variable.

sshaw commented 2 years ago

btw do you think this should be enable using a configuration variable? So default behaviour(which includes the "bug") won't be affected when someone update the package and they can activate it later by customizing that variable.

We have 3 scenarios in which the plain=1 parameter can affect usage:

  1. Link to "markup file" with line numbers
  2. Link to "markup file" without line numbers
  3. Link to a non "markup file" that results in "unexpected behavior" due to GitHub special handling, e.g., CSV

In the case of 1 & 2 I think we can assume reasonable defaults of:

  1. use plain=1
  2. don't use plain=1

I could see the config var addressing 1 & 2 if the default was to always use plain=1 but from a usability perspective its usage is awkward: user must set the config variable, call git-link interactively, then set the config variable back to its default prior value. Excluding plain=1 would be better handled via a prefix arg but this case remains to be seen and I'd be fine with 2 as specified. If people have use cases for no line numbers + plain=1 this default can be reassessed then.

So... it seems that the configuration variable you're purposing would be to address 3, if 3 even exists? Really seems that in this scenario we shouldn't use plain=1. As far as we know now it's a noop, right? So why even include it?

sshaw commented 2 years ago

So... it seems that the configuration variable you're purposing would be to address 3, if 3 even exists? Really seems that in this scenario we shouldn't use plain=1. As far as we know now it's a noop, right? So why even include it?

I can already see people opening issues just due to the presence of plain=1 for a link that would never be rendered "not plain".

erickgnavar commented 2 years ago

sorry for the delay in the response, personal issues :(, I think that to cover the points you described we can have a list of markup extensions that shouldn't be rendered, for example .org, .md and so on, and add plain=1 only for those files, this list can be a defcustom variable so in case the user wants to disable any of those it will be easier.

This solution will be complementary with the defaults you proposed, so in case the file belongs to the cases in the list(org, md, etc) we use those defaults otherwise we just avoid plain=1

sshaw commented 2 years ago

I think that to cover the points you described we can have a list of markup extensions that shouldn't be rendered,

List should be those that should be rendered My mistake. Your statement above is correct.

for example .org, .md and so on, and add plain=1 only for those files, this list can be a defcustom variable so in case the user wants to disable any of those it will be easier.

Yes, sounds great!

Do you think it's best to keep github- out of the variable name so that it can be applied to other services if/when they support similar functionality? (GitLab does, but not sure if they have query string param).

erickgnavar commented 2 years ago

Do you think it's best to keep github- out of the variable name so that it can be applied to other services if/when they support similar functionality? (GitLab does, but not sure if they have query string param).

Yes, the variable has a generic name, so we can add support for other services in the future :)

sshaw commented 2 years ago

Thanks so much. Excited to have this addition 🎆