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

Fix added reserved keywords to DotNetNormalizer.cs #59

Merged
merged 5 commits into from
Oct 14, 2021

Conversation

badcommandorfilename
Copy link
Contributor

Added string and this to C# reserved keyword list

Hi, I was using this library with tensorflow.js and had to add these keywords to work with the JS names used in that library.

This is pretty low-hanging fruit, so I'm happy to extend this with more of the reserved names in C# if you're looking for help with contributions.

Added string and this to C# reserved keyword list
@canhorn
Copy link
Owner

canhorn commented Oct 12, 2021

@badcommandorfilename Thanks for adding these normalization keywords!
Can you add to the scenario testing, the two files are below that test this for Accessors and Properties:
https://github.com/canhorn/EventHorizon.Blazor.TypeScript.Interop.Generator/blob/05dedfdd7469b2ada3fc210df7d8496f53fe82ef/Tests/EventHorizon.Blazor.TypeScript.Interop.Generator.Tests/GenerateClassStatementStringTests/Accessors/Scenarios/DotNetNormalized.ts
https://github.com/canhorn/EventHorizon.Blazor.TypeScript.Interop.Generator/blob/05dedfdd7469b2ada3fc210df7d8496f53fe82ef/Tests/EventHorizon.Blazor.TypeScript.Interop.Generator.Tests/GenerateClassStatementStringTests/Properties/Scenarios/DotNetNormalized.ts

You will have to update the corresponding DotNetNormalized.Expected.txt that will be next to the files. These are the source code from TypeScript and the generated C# that is expected to be generated.

@badcommandorfilename
Copy link
Contributor Author

@canhorn I've updated the tests with basic coverage - I found another one at https://github.com/canhorn/EventHorizon.Blazor.TypeScript.Interop.Generator/blob/05dedfdd7469b2ada3fc210df7d8496f53fe82ef/Tests/EventHorizon.Blazor.TypeScript.Interop.Generator.Tests/GenerateClassStatementStringTests/Constructors/DotNetNormalizedArguments.ts

I'm not sure if you prefer to squash commits before merging, so let me know if there are any more changes.

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #59 (d41dab8) into main (05dedfd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head d41dab8 differs from pull request most recent head feaeb4f. Consider uploading reports for the commit feaeb4f to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main      #59   +/-   ##
=======================================
  Coverage   98.85%   98.85%           
=======================================
  Files          92       92           
  Lines        4275     4278    +3     
  Branches      384      384           
=======================================
+ Hits         4226     4229    +3     
  Misses         18       18           
  Partials       31       31           
Impacted Files Coverage Δ
....Interop.Generator/Normalizers/DotNetNormalizer.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05dedfd...feaeb4f. Read the comment docs.

@canhorn
Copy link
Owner

canhorn commented Oct 13, 2021

@badcommandorfilename Look Good!
The checks failed, but that was because of missing secrets, that is expected. I will fix that in the future so it will not run those tasks when its a external PR.

Also I noticed that the author on some of the commits are under another name. If you squash the commit under your preferred name that should fix the mixed authors. But it is up to you.

I am fine to merge this PR either way. Let me know. :)

@badcommandorfilename
Copy link
Contributor Author

Thanks for spotting the author name @canhorn - I've fixed it up now and I'm happy to merge.

This is a very cool project, by the way! Thanks for making something like this. I'm very happy to contribute any more fixes or features as I keep using it.

@canhorn canhorn merged commit 4a919bf into canhorn:main Oct 14, 2021
@canhorn
Copy link
Owner

canhorn commented Oct 14, 2021

Thanks!
Anymore contributions are welcomed!

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

Successfully merging this pull request may close these issues.

None yet

2 participants