Skip to content

[clang-doc] --repository links don't work with github #59814

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

Closed
danakj opened this issue Jan 4, 2023 · 4 comments · Fixed by #131280
Closed

[clang-doc] --repository links don't work with github #59814

danakj opened this issue Jan 4, 2023 · 4 comments · Fixed by #131280
Assignees

Comments

@danakj
Copy link
Contributor

danakj commented Jan 4, 2023

The generated links are of the form https://repository/path.h#22

However github requires the URL fragmment to be L22 not 22, so it should be https://repository/path.h#L22

A configuration option to adjust the url fragment would be appreciated.

ilovepi added a commit that referenced this issue Jan 11, 2025
The current check in writeFileDefinition() is incorrect, and prevents us
from ever emitting the URL from the clang-doc tool. The unit tests do
test this, but call the API directly circumventing the check.

This is the first step towards addressing #59814.
ilovepi added a commit that referenced this issue Jan 13, 2025
The current check in writeFileDefinition() is incorrect, and prevents us
from ever emitting the URL from the clang-doc tool. The unit tests do
test this, but call the API directly circumventing the check.

This is the first step towards addressing #59814.
ilovepi added a commit that referenced this issue Feb 6, 2025
The current check in writeFileDefinition() is incorrect, and prevents us
from ever emitting the URL from the clang-doc tool. The unit tests do
test this, but call the API directly circumventing the check.

This is the first step towards addressing #59814.
ilovepi added a commit that referenced this issue Feb 6, 2025
The current check in writeFileDefinition() is incorrect, and prevents us
from ever emitting the URL from the clang-doc tool. The unit tests do
test this, but call the API directly circumventing the check.

This is the first step towards addressing #59814.
ilovepi added a commit that referenced this issue Feb 6, 2025
The current check in writeFileDefinition() is incorrect, and prevents us
from ever emitting the URL from the clang-doc tool. The unit tests do
test this, but call the API directly circumventing the check.

This is the first step towards addressing #59814.
Icohedron pushed a commit to Icohedron/llvm-project that referenced this issue Feb 11, 2025
The current check in writeFileDefinition() is incorrect, and prevents us
from ever emitting the URL from the clang-doc tool. The unit tests do
test this, but call the API directly circumventing the check.

This is the first step towards addressing llvm#59814.
@hulxv
Copy link
Member

hulxv commented Mar 11, 2025

@ilovepi Can I be assigned to this issue? Please give me more details about it also.

@hulxv
Copy link
Member

hulxv commented Mar 11, 2025

As I read in 1a7e79b, we can parse the URL of the repo and detect if it's a host that has a prefix or not, such as GitHub or Gitlab (they also use L letter for lines as github). Is my approach correct?

@ilovepi
Copy link
Contributor

ilovepi commented Mar 11, 2025

@hulxv So the basic problem seems to be that the current implementation assumes how this will be spelled, which is problematic. To start, I'd suggest adding a configuration option to set this, via command line option, and then follow up with some kind of detection heuristic in a second patch.

@hulxv
Copy link
Member

hulxv commented Mar 11, 2025

To start, I'd suggest adding a configuration option to set this, via command line option, and then follow up with some kind of detection heuristic in a second patch.

Do you mean adding an argument to change the line anchor manually from the user? like --repo-line-prefix=L

My first idea is doing something like that, and listing prefixes of each host. Is it not better?

static std::string getLineAnchor(const llvm::StringRef RepositoryUrl,
                                 int LineNumber) {

  const std::unordered_map<std::string, std::string> SpecialRepoAnchorMap = {
      {"github.com", "L"},
      {"gitlab.com", "L"},
      // etc....
  };

  for (const auto &entry : SpecialRepoAnchorMap) {
    if (RepositoryUrl.contains(entry.first))
      return "#" + entry.second + std::to_string(LineNumber);
  }

  return "#" + std::to_string(LineNumber); // default prefixe
}

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

Successfully merging a pull request may close this issue.

4 participants