moodmosaic / Fare

Port of Java dk.brics.automaton and xeger, mostly used for generating strings that match a specific regular expression.
http://www.brics.dk/automaton/
MIT License
183 stars 43 forks source link

Add .NET Core support, update build scripts #30

Closed zvirja closed 6 years ago

zvirja commented 6 years ago

Closes #21

It's a HUGE rework for the project, as it would be hard to make those changes granular. The main changes applied in the PR:

The code logic has not been changed somehow during the refactoring.

To simplify the review process I'd suggest you to checkout the branch from my fork. The diff review will be quite complex and a lot of things changed.


Further steps:

  1. Review the changes and address concerns
  2. Enable AppVeyor support:
    • Add AppVeyor configuration based on your Git account
    • Encrypt the NuGet push key via AppVeyor and put it to the appveyor.yml file to be able to trigger release via creation of tag in GitHub UI. You can encrypt it and provide the value here, so I'll update the file as a part of this PR.
  3. Push a new minor version of the package as nothing changed in the API.

@moodmosaic Looking forward to your review and thanks for your attention in advance ๐Ÿ˜‰

moodmosaic commented 6 years ago

@zvirja, this is awesome, thank you! :heart: :rocket: :+1: :100: If we go ahead and merge this now, does it mean we won't be able to release new versions to NuGet Gallery unless we go and push git tags on GitHub?

zvirja commented 6 years ago

does it mean we won't be able to release new versions

No at all :) You can still do that locally:

The only issue is that if you have AppVeyor configured, it will also try to publish the artifacts when you push the tag (and will fail, as NuGet doesn't allow to overwrite packages). Therefore, if CI is configured, it's easier to delegate that work to it.

In fact, it could happen that CI is unable to push packages. I've already had such situation with AutoFixture - NuGet was down and I didn't know about that. Therefore, I performed the local build (or downloaded artifacts from the AppVeyor, I don't remember) and pushed the missing packages manually to the NuGet Gallery later.

So as you can see, AppVeyor simplifies the publish process, but doesn't prevent you from the manual publish if you wish ๐Ÿ˜‰

moodmosaic commented 6 years ago

Sweet! And so the NuGet Package will now target both runtimes, .NET 3.5 and .NET Standard 1.1, right?

(By the way, I think I should switch, in the future, to newer versions for both .NET and .NET Standard...)

zvirja commented 6 years ago

And so the NuGet Package will now target both runtimes, .NET 3.5 and .NET Standard 1.1, right?

Exactly :wink:

I think I should switch, in the future, to newer versions for both .NET and .NET Standard

Personally, I don't see any reason to do that. This library is so simple, that I was able to target netstandard 1.1 - it doesn't require any extra API :hushed: Well, NET 3.5 looks a bit outdated, but I don't see any benefits from the update :)

zvirja commented 6 years ago

@moodmosaic Do you have any plans to setup AppVeyor? ๐Ÿ˜‰

moodmosaic commented 6 years ago

I'd like to, but I don't have time to do it. I'm extremely busy with work this period.

zvirja commented 6 years ago

Awesome! Sure, do it when you have more free time. ๐Ÿ˜‰

In the meanwhile, could you please arrange the tags and push a new version to the NuGet manually? It will enable me to consume it in AutoFixture โ˜บ๏ธ

moodmosaic commented 6 years ago

In the meanwhile, could you please arrange the tags and push a new version to the NuGet manually? It will enable me to consume it in AutoFixture โ˜บ๏ธ

Done in https://github.com/moodmosaic/Fare/issues/29#issuecomment-358944318

zvirja commented 6 years ago

It seems that you marked a new version with 1.0.5. But I'd rather say 1.1.0 - as a new feature appeared in the product - support of .NET Core ๐ŸŽ‰ Do you have some different opinion about that?

moodmosaic commented 6 years ago

Oops, that was a mistake on my part. Good catch.

moodmosaic commented 6 years ago

@zvirja, it's fixed now.