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

added net4.8 support #131

Merged
merged 4 commits into from
Jun 24, 2024
Merged

added net4.8 support #131

merged 4 commits into from
Jun 24, 2024

Conversation

ankitdas13
Copy link
Contributor

Added Support for .net 4.8

@ankitdas13 ankitdas13 requested a review from shwatang June 17, 2024 11:41
Razorpay.nuspec Outdated Show resolved Hide resolved
Comment on lines 8 to 13
[assembly: AssemblyTitle("RazorpayClient48")]
[assembly: AssemblyDescription("")]
[assembly: AssemblyConfiguration("")]
[assembly: AssemblyCompany("")]
[assembly: AssemblyProduct("RazorpayClient48")]
[assembly: AssemblyCopyright("Copyright © 2024")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[assembly: AssemblyTitle("RazorpayClient48")]
[assembly: AssemblyDescription("")]
[assembly: AssemblyConfiguration("")]
[assembly: AssemblyCompany("")]
[assembly: AssemblyProduct("RazorpayClient48")]
[assembly: AssemblyCopyright("Copyright © 2024")]
[assembly: AssemblyTitle("Razorpay")]
[assembly: AssemblyDescription("Razorpay SDK for .Net")]
[assembly: AssemblyConfiguration("")]
[assembly: AssemblyCompany("Razorpay Software Private Limited")]
[assembly: AssemblyProduct("Razorpay.API")]
[assembly: AssemblyCopyright("Copyright © 2015 - 2024")]

Should't it be this? Correct me if mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

[assembly: AssemblyTitle("Razorpay")]

<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="Newtonsoft.Json" version="13.0.3" targetFramework="net48" />
<package id="Portable.BouncyCastle" version="1.8.0" targetFramework="net48" />
Copy link
Contributor

Choose a reason for hiding this comment

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

why changed it from 1.9.0 version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

<package id="Portable.BouncyCastle" version="1.9.0" targetFramework="net48" />

@@ -0,0 +1,199 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
Copy link
Contributor

Choose a reason for hiding this comment

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

why we not adding DefaultTargets key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

<Project ToolsVersion="15.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

<AssemblyName>Razorpay</AssemblyName>
<TargetFrameworkVersion>v4.8</TargetFrameworkVersion>
<FileAlignment>512</FileAlignment>
<Deterministic>true</Deterministic>
Copy link
Contributor

Choose a reason for hiding this comment

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

What this does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

<StartupObject />
</PropertyGroup>
<ItemGroup>
<Reference Include="crypto, Version=1.8.0.0, Culture=neutral, PublicKeyToken=0e99375e54769942, processorArchitecture=MSIL">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Include="crypto the correct way? Shouldnt it be BouncyCastle.Crypto ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

<Reference Include="BouncyCastle.Crypto, Version=1.9.0.0, Culture=neutral, PublicKeyToken=0e99375e54769942, processorArchitecture=MSIL">

Comment on lines 38 to 41
<HintPath>packages\Portable.BouncyCastle.1.8.0\lib\net46\crypto.dll</HintPath>
</Reference>
<Reference Include="Newtonsoft.Json, Version=13.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">
<HintPath>packages\Newtonsoft.Json.13.0.3\lib\net45\Newtonsoft.Json.dll</HintPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please once check the HintPath and confirm if its correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its correct

Screenshot 2024-06-20 153132

Comment on lines +44 to +52
<Reference Include="System.IO.FileSystem, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">
<HintPath>packages\System.IO.FileSystem.4.0.0\lib\net46\System.IO.FileSystem.dll</HintPath>
<Private>True</Private>
<Private>True</Private>
</Reference>
<Reference Include="System.IO.FileSystem.Primitives, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">
<HintPath>packages\System.IO.FileSystem.Primitives.4.0.0\lib\net46\System.IO.FileSystem.Primitives.dll</HintPath>
<Private>True</Private>
<Private>True</Private>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why including this as part this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to use System.Net.HttpWebRequest namespace in net4.8 we need to add reference.

Copy link
Contributor

@shwatang shwatang left a comment

Choose a reason for hiding this comment

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

Please address the comments. Also, please do confirm if this change has been tested thoroughly.

@ankitdas13
Copy link
Contributor Author

Please address the comments. Also, please do confirm if this change has been tested thoroughly.

I addressed these comments and made changes, as well as testing it

@ankitdas13 ankitdas13 requested a review from shwatang June 20, 2024 10:13
Copy link
Contributor

@shwatang shwatang left a comment

Choose a reason for hiding this comment

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

LGTM

@ankitdas13 ankitdas13 added the TestingNotRequired TestingNotRequired label for BVT label Jun 24, 2024
@ankitdas13 ankitdas13 merged commit 6316b64 into master Jun 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TestingNotRequired TestingNotRequired label for BVT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants