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 .net project & file references #789

Merged
merged 6 commits into from
Jan 29, 2020

Conversation

xt0rted
Copy link
Member

@xt0rted xt0rted commented Jan 25, 2020

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

Adds support for project and file references inside of csproj and friends. It supports both the classic and newer sdk style formats.

Linkifying files might not be the best on large projects since you could potentially have tens of thousands of files, but that's only an issue when using the classic style. In my own testing of a project with 1,006 added links I didn't notice any additional slow down but I'm also using a core i7 6900k with 64gb of ram so this might not be the best test machine.

Project references

File references

Closes #450

Copy link
Member

@stefanbuck stefanbuck left a comment

Choose a reason for hiding this comment

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

I'm much less worried about performance now that #774 is in place. On larger project the tree API call may does not contain all files .. we will see. I added two small comments, apart from the looks good to me.

packages/helper-grammar-regex-collection/index.js Outdated Show resolved Hide resolved

resolve(path, [target]) {
// Both / and \ are supported as the path separator so we need to normalize it to /
target = target.replace(/\\/g, '/');
Copy link
Member

Choose a reason for hiding this comment

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

Just asking myself whether this functionality should be part of relativeFile so other plugins can benefit from it as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that too. There's also a number of spots doing what relativeFile does manually, not sure if any of them would benefit from this though.

Copy link
Member

Choose a reason for hiding this comment

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

Ohh I wasn't aware of this. Maybe worth tackling this in a separate Pull Request at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I add this into relativeFile or do that in a separate PR with #796?

Copy link
Member

Choose a reason for hiding this comment

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

I would tend to do this in a separate PR, but feel free to make it part of this PR.

Comment on lines 33 to 34
<!-- @OctoLinkerResolve(Directory.Build.props) -->
<Compile Include="../Directory.Build.props" />
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 what value should these resolve to? The test results seem to be pointing to nuget.org instead of github.com.

Copy link
Member

Choose a reason for hiding this comment

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

If these links are meant to be relative the annotation needs to be
// @OctoLinkerResolve(<root>/dotnet/Directory.Build.props)

<root> will be replace with https://github.com/OctoLinker/OctoLinker/tree/e2e/fixtures when generate the test data out of all fixtures.

However, this does not explain why the test is pointing to nuget.org.

Copy link
Member

Choose a reason for hiding this comment

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

Strange, I checkout out this PR and hooked up everything locally. Navigating to

<ProjectReference Include="../Classic/Project.csproj" />
does not link this line at all.

Copy link
Member

Choose a reason for hiding this comment

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

Well the casing of /Classic/ is different on disk. However, the next include does not work either

<Compile Include="../Directory.Build.props" />

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 path casing was wrong. Classic should have been all lowercase. Switching that fixes it.

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 tests are passing now for these, but some other ones are failing that aren't related to this change 😕

Copy link
Member

Choose a reason for hiding this comment

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

For some odd reason, E2E tests are timing out way more often on PRs than on master. Let's get this merged and we will see what happens 😉

Copy link
Member

Choose a reason for hiding this comment

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

It's passing 😄

@xt0rted xt0rted marked this pull request as ready for review January 29, 2020 22:19
@stefanbuck stefanbuck merged commit 411067e into OctoLinker:master Jan 29, 2020
@xt0rted xt0rted deleted the dotnet-project-references branch January 29, 2020 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Adding relative project linking for .net projects
2 participants