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

Psychrolib 2.2.0 #42

Merged
merged 14 commits into from
Nov 5, 2019
Merged

Psychrolib 2.2.0 #42

merged 14 commits into from
Nov 5, 2019

Conversation

dmey
Copy link
Contributor

@dmey dmey commented Oct 31, 2019

This PR bumps the version to 2.2.0 in all src files, adds a changelog CHANGELOG.txt.

@didierthevenard and @DJGosnell can you please let me know what you think about how I rephrased the README? Perhpas we may want to rephrase if we need to show that PsychroLib for .NET has been written in C# and not F# or Visual Basic...

Note to self merge when #41 is closed.

@dmey dmey added this to the 2.2.0 milestone Oct 31, 2019
@dmey dmey self-assigned this Oct 31, 2019
@dmey dmey mentioned this pull request Oct 31, 2019
5 tasks
README.md Outdated
@@ -19,12 +19,12 @@ Examples on how to use PsychroLib in all the supported languages are described i

- Python: from the command prompt with `pip install psychrolib`.

- C#, F#, and Visual Basic: from the [NuGet package](https://www.nuget.org/packages/PsychroLib/) manager or clone the repository, and bundle according to your requirements.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the language the .NET port is written only in C#, it may be clearer to word it similar below:

- C#, F#, and Visual Basic: from the [NuGet package](https://www.nuget.org/packages/PsychroLib/) manager.
- C#: clone the repository, and bundle according to your requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍. Split it into .NET and C# -- see 91e2166

@DJGosnell
Copy link
Contributor

Outside of the one comment I have above to your question about language wording, LGTM.

Copy link
Contributor

@didierthevenard didierthevenard left a comment

Choose a reason for hiding this comment

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

I don't want to sound negative because I think that the addition of C# is great, but I think the mention of F# and VB.net is a bit of a stretch. We provide the source code in Python, Fortran, etc., and C#. That is, if I am familiar with a particular language in the list above and I want to work with the source code of PsychroLib, I can do so. I cannot do so with F# or VB.net because there is no F# or VB.net version. What about something like this:
Versions of PsychroLib are available for Python, C, Fortran, JavaScript, Microsoft Excel Visual Basic for Applications (VBA), and C#. The C# version works in the .NET environment and therefore is also callable from programs written in F# or Visual Basic.

@DJGosnell
Copy link
Contributor

@didierthevenard The more that I think about it, the more I agree. The .NET code is written in C#, compiled to the .NET IL and can be used from F# & VB.net, but the source is obviously not written in their respective languages. I have no intention on porting the library to those languages and would not want someone asking for support in those languages since it said in the repository F# & VB.net.

The NuGet package can be used from those other languages and if someone is looking for a library like this to use in their C$, F# or VB.net application, they will already know how to use NuGet packages as they are common in all three .NET languages.

If you want to pull all references to F# and VB.net, I am fine with that because it simplifies the explanation of the project and we are not loosing any inherent compatibility with the other languages via the NuGet package.

How does this sound?

@didierthevenard
Copy link
Contributor

That sounds good to me, and is simpler. @dmey do you agree?

@dmey
Copy link
Contributor Author

dmey commented Nov 4, 2019

@didierthevenard and @DJGosnell sounds good! I have rephrased in dc50cc8, removing refrerences to .NET, F# and VB. The only references to F# and VB are under the Installing section in the README. How does it look?

@DJGosnell
Copy link
Contributor

DJGosnell commented Nov 4, 2019

I'm honestly thinking that dropping all VB.net and F# references is the better call.

I think that reverting to the original will be better by itself and leave out the line about F# and VB in the readme.md.

- C#: from the [NuGet package](https://www.nuget.org/packages/PsychroLib/) manager or clone the repository, and bundle according to your requirements.

I originally was thinking about increasing this project's visibility to the other language's communities, but now I think it may just add unwanted support/help requests.

@dmey
Copy link
Contributor Author

dmey commented Nov 4, 2019

Sounds good with me but how about having .NET in parethesis after C# like:

  • C# (.NET): from the NuGet package manager or clone the repository, and bundle according to your requirements.

Anyone using .NET should know that they can use it in F# and VBA as well. But let me know -- happy to remove that too! 😄

@DJGosnell
Copy link
Contributor

Sounds good with me but how about having .NET in parethesis after C# like:

  • C# (.NET): from the NuGet package manager or clone the repository, and bundle according to your requirements.

Anyone using .NET should know that they can use it in F# and VBA as well. But let me know -- happy to remove that too! 😄

Agreed.

I believe that the CHANGELOG.txt will also need to be updated accordingly.

Add C# support with .NET Standard 1.0 (.NET Core >= 1.0 & .NET Framework >= 4.5).

Didn't want to add confusion in the process here but I think this is all much clearer now.

@dmey
Copy link
Contributor Author

dmey commented Nov 4, 2019

Great thanks! 👍 Now changed in 1e5bcbe.

@didierthevenard please let me know if you are happy and I will merge it in.

@didierthevenard
Copy link
Contributor

Looks good - thank you both for your work on this!

@dmey dmey merged commit ab71aef into master Nov 5, 2019
@dmey dmey deleted the 2.2.0 branch November 5, 2019 07:37
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.

None yet

3 participants