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

Add support for C++ project files and C & C++ files #1048

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

xt0rted
Copy link
Member

@xt0rted xt0rted commented Oct 2, 2020

Pull Request Badge

Checklist:

  • If this PR is a new feature, please provide at least one example link
  • Make sure all of the significant new logic is covered by tests

Example files:

Working on this made me realize the dot-net-project plugin should probably be renamed to msbuild since all of the files it handles are msbuild files and not specifically .net related.

@xt0rted xt0rted changed the title Add support for c++ project files, and C & C++ files Add support for C++ project files and C & C++ files Oct 2, 2020
},

getLinkRegexes() {
return [C_INCLUDE];
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind adding an E2E test for this plugin as well. It doesn't have to be an E2E test for every single file extensions, maybe just a single one for .cpp? What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, there's a few things missing from this one still. I'll get it finished up over the weekend.

@stefanbuck
Copy link
Member

Working on this made me realize the dot-net-project plugin should probably be renamed to msbuild since all of the files it handles are msbuild files and not specifically .net related.

I agree, although it's not a huge problem, since the name is not user facing and mainly just confusing for contributes (maybe)

<ClCompile Include="foo.cpp" />

<!-- @OctoLinkerDoesNotResolve -->
<ClCompile Include="$(OutDir)strings.cpp">
Copy link
Member Author

Choose a reason for hiding this comment

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

@stefanbuck this test failed because it didn't resolve the dependency, and when that happens OctoLinker still injects an <a> but doesn't set the href attribute. How do we want to handle that scenario?

This is what the html looks like for this line:

<td class="blob-code blob-code-addition">
  <span class="blob-code-inner blob-code-marker" data-code-marker="+">
    &lt;
    <span class="pl-ent">ClCompile</span>
    <span class="pl-e">Include</span>
    =
    <span class="pl-s">
      <span class="pl-pds">"</span>
      <a data-pjax="true" class="octolinker-link">$(OutDir)strings.cpp</a>
      <span class="pl-pds">"</span>
    </span>
    &gt;
  </span>
</td>

Copy link
Member Author

Choose a reason for hiding this comment

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

The false positives are resolved in #1069. Will rebase & finish up once that fix gets merged.

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

Successfully merging this pull request may close these issues.

support for C/C++
2 participants