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

Add Varint and README code unit tests #79

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ed-flanagan
Copy link
Contributor

@ed-flanagan ed-flanagan commented Sep 26, 2021

This PR has a couple of changes, split out across commits. Happy to drop/split out any commits if felt PR is too big.

  • Add unit tests for VarintTranslator methods
  • Update README deck code example
    • Update README deck code to reflect current sorting strategy. The set/faction orders were flipped.
    • Add to unit test data file
  • Visual studio code auto-formatted some existing code in UnitTest1.cs
  • Update LoRDeckCodes_Tests.csproj ProjectReference (s/Lor/LoR/)
  • Opinionated:
    • Re-ordered UnitTest1.cs using directives
    • Removed unused Newtonsoft.Json package
    • Removed byte order mark from tracked files
    • Try to make *.csproj spacing consistent
    • Update netcoreapp to v3.1
      • netcoreapp2.0 used in both projects. This version is EOL. Update the LTS version 3.1
        (2022-12-03)
      • Resolves Package warning message: warning NETSDK1138: The target framework 'netcoreapp2.0' is out of support and will not receive security updates in the future.
      • Happy to update to .NET 6, but figured this was the smallest jump https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core

@ed-flanagan ed-flanagan changed the title Add Varint unit tests Add Varint and README code unit tests Oct 24, 2021
@ed-flanagan
Copy link
Contributor Author

@RiotTuxedo 👋 sorry for the ping. I saw others do it and this PR has been sitting for a while.
Any thoughts? I can rebase with recent changes that merged, but general idea’s the same

@RiotTuxedo
Copy link
Contributor

Hey ed,

I'm focusing mainly on helping to keep the documentation up-to-date. The lor engineers handle updating the repo with the logic to support new deck codes and the tests. They'd have to review your changes and decide if they want to accept it into the code. I'll let one of them know there's an open PR, but there's no guarantees they'd have time to review and merge it.

@ed-flanagan
Copy link
Contributor Author

👍 sg, no worries.
I was looking at changes in #88 which had some small overlap, so figured I’d check in.

@RiotTuxedo
Copy link
Contributor

Ah makes sense. That's one of the lor devs that didn't have access to merge the changes for the latest set.

@ed-flanagan
Copy link
Contributor Author

Ah right on, appreciate the context - sorry again for the ping

ProjectReference Include filename was 'LorDeckCodes.csproj'.
Change to 'LoRDeckCodes.csproj' (capitalize first R)
* netcoreapp2.0 used in both projects. This version is EOL.
  Update the LTS version 3.1 (2022-12-03)
* Resolve Package warning message:
  "warning NETSDK1138: The target framework 'netcoreapp2.0' is out of
  support and will not receive security updates in the future."
* Try and make csproj spacing consistent
* Current Varint implementation is unsigned LEB128, reading the
  least significant bytes first.
  Impl. adapted from https://github.com/topas/VarintBitConverter
  which references
  https://developers.google.com/protocol-buffers/docs/encoding#varints
* Update the root README to reflect current implementation
* Add unit tests for VarintTranslator
* Update README deck code to reflect current
  sorting strategy. The set/faction orders
  were flipped.
* Add to unit test data file
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