Skip to content
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

Load documentationUri from Bicep registry module index data #12207

Merged
merged 3 commits into from Oct 27, 2023

Conversation

shenglol
Copy link
Contributor

@shenglol shenglol commented Oct 20, 2023

The AVM module paths use a different convention than other public registry modules, so we cannot rely on a fixed module path format to generate documentUri for modules when creating completions for public registry module paths and versions. Now that documentUri is added to the public registry module index data, we should use it directly.

The PR also fixes a bad HttpClient usage pattern (using var httpClient = new HttpClient()) that can lead to exhaustion of available TCP ports by leveraging Typed Clients with IHttpClientFactory .

Closes #12122.

Microsoft Reviewers: Open in CodeFlow

@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2023

Test this change out locally with the following install scripts (Action run 6671150695)

VSCode
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 6671150695
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 6671150695"
Azure CLI
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 6671150695
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 6671150695"

@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2023

Test Results

     132 files  ±  0       132 suites  ±0   4h 27m 2s ⏱️ + 20m 2s
10 670 tests  -   3  10 670 ✔️  -   3  0 💤 ±0  0 ±0 
51 567 runs   - 12  51 567 ✔️  - 12  0 💤 ±0  0 ±0 

Results for commit 63240e1. ± Comparison against base commit 4137ecc.

♻️ This comment has been updated with latest results.


private const string LiveDataEndpoint = "https://aka.ms/BicepModulesMetadata";
private readonly TimeSpan CacheValidFor = TimeSpan.FromHours(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want you to bother changing this back, but why add all the whitespace? This style doesn't improve readability, while grouping by similarity IMHO does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a convention that I'm used to (not sure where I learned it from, maybe ReSharper or StyleCop), and I find it slightly more readable with line breaks between fields. I'm have no objections to removing the line breaks, but personally I'm not a fan of grouping fields by similarity, and I'd follow https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1201.md for sorting class members.

@shenglol shenglol merged commit c06a72a into main Oct 27, 2023
47 checks passed
@shenglol shenglol deleted the shenglol/load-documentation-uri-from-br-index branch October 27, 2023 18:56
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.

Fix documentation link for AVM modules in completion widget
2 participants