Skip to content
This repository has been archived by the owner on Aug 5, 2024. It is now read-only.

Put C# port under solution #24

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

rolshevsky
Copy link

@rolshevsky rolshevsky commented Jul 20, 2018

The reason of this PR is moving all C# related files under solution, as a target I used .netstandard2.0.

Benefits:

  • netstandard as a target
  • simplified process of making changes in source code
  • better approach for running tests (Unit + Performance(BenchmarkDotNet))
  • **can be shipped as a NuGet package in future

Source code of core functionality I left as-is.

View of Solution explorer:
rider64_2018-07-20_21-52-04

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@rolshevsky rolshevsky changed the title C Put C# port under solution Jul 20, 2018
@rolshevsky
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@NeilFraser
Copy link
Contributor

Hi, thanks for this PR.

The main issue here is how to support the existing developers who depend on the DMP library. They have automated build scripts that pull DMP into their projects. Wouldn't this PR break all existing developers?

@jhgbrt
Copy link

jhgbrt commented Jul 31, 2018

@rolshevsky if you're interested, I am maintaining a 'canonical C#' version of the original C# version at https://github.com/jhgbrt/google-diff-match-patch-csharp/.

@mattkoch614
Copy link

I'd like to see this merged, if possible. Would the potential breakage for other developers be remedied if instead of moving the csharp file, it stayed where it was?

@rolshevsky
Copy link
Author

@mattkoch614 @NeilFraser , I think, for saving compatibility I can restore DiffMatchPatch.cs file in original path, and in this case it will not break usage flow for existing developers. Then we can mark this file as deprecated in header (comment or something) and continue development in solution project.
Is it make sense?

@jhgbrt
Copy link

jhgbrt commented Sep 21, 2018

@rolshevsky

It is possible to add the sln and csproj files without breaking the folder structure:

  • add a .csproj in the root project for the core library, excluding the tests folder in the xml:
<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <DefaultItemExcludes>$(DefaultItemExcludes);tests\**</DefaultItemExcludes>
  </PropertyGroup>
</Project>

  • Add a separate csproj file for the tests in the corresponding tests folder
    in the root folder, add the sln file, and add both csproj files to that sln file

@mattkoch614
Copy link

mattkoch614 commented Sep 21, 2018

@jhgbrt 's suggestion sounds reasonable. My main reason for wanting this merged is to have an easy way of creating a *.nupkg file so this library can be hosted on Nuget. See #36

EDIT: I added my own PR to resolve these issues. #47

@rolshevsky
Copy link
Author

@NeilFraser, @jhgbrt, @mattkoch614 - I was updated PR with the following changes:

  • I restored DiffMatchPatch.cs in original folder by original path: csharp/DiffMatchPatch.cs, so we have no any braking changes anymore for existing users.
  • I put this original DiffMatchPatch.cs out of solution. It will keep backward compatibility, and all further development supposed to be done from under solution.
  • I marked this file and all public classes inside as Obsolete. In future, when we will get NuGet support we can add some more informative messages that will say that user can switch from this old version to NuGet package.

@mattkoch614 I have some doubts about #47 where we included original DiffMatchPatch.cs into solution, because as it was mentioned by @NeilFraser we need to keep backward compatibility for existing users. With current approach we keeping original file as-is outside the solution.

@mattkoch614
Copy link

@rolshevsky Thanks for the information.

@rolshevsky @jhgbrt @NeilFraser
Correct me if I am wrong, but isn't the whole point to have the C# library added to a .NET project and solution while keeping file paths unchanged? How would this break compatibility for anyone? Any scripts that exist that pull down the DiffMatchPatch.cs file would theoretically still function. All #47 does is wrap existing files (without moving them) into a project and solution with some added metadata for an eventual Nuget package.

Forgive me if I am missing something.

@rolshevsky
Copy link
Author

rolshevsky commented Oct 2, 2018

@mattkoch614

Any scripts that exist that pull down the DiffMatchPatch.cs file would theoretically still function.

Yeah, exactly, I mean if we left path as-is and then start patching the library API (#25) - this will be a breaking change for existing users. But in case if we just deprecate existing DiffMatchPatch.cs we can keep making changes in project from under solution and just bumping NuGet package version, and in this case it will not affect existing users.

But again, it's just my understanding of the problem here. @NeilFraser please, correct me if I'm wrong.

@Poltuu
Copy link

Poltuu commented Jul 12, 2020

I would be pretty useful to have a more modern c# version of this. It seems to me that, if we want to assume full compatibility, the only way forward is to use the existing cs file as-is with a solution/project.

@rolshevsky
Copy link
Author

@Poltuu to some extends, it is implemented in that way already. I left original .cs file as is to be fully compatible. Under solution I put a copy of the original .cs file and for solution of course we can add any kind versioning we need (semver etc.), and then we can publish it on a NuGet as well.

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

Successfully merging this pull request may close these issues.

6 participants