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

use test tokens from python lib #36

Merged
merged 3 commits into from
Nov 7, 2024
Merged

use test tokens from python lib #36

merged 3 commits into from
Nov 7, 2024

Conversation

beesaferoot
Copy link
Collaborator

@beesaferoot beesaferoot commented Nov 6, 2024

Brief summary of the change made

With this change, the tokens generated now match the current python library version. Closes #35

Note: I converted the jsonl file from the python repo to json for convenience.

Are there any other side effects of this change that we should be aware of?

I noticed few of the sample test cases had value_raw as null which results in a crash (not a defined behaviour)

Describe how you tested your changes?

Pull Request checklist

Please confirm you have completed any of the necessary steps below.

  • Meaningful Pull Request title and description
  • Changes tested as described above
  • Added appropriate documentation for the change.
  • Created GitHub issues for any relevant followup/future enhancements if appropriate.

@beesaferoot beesaferoot requested a review from dmohns November 6, 2024 22:39
@dmohns
Copy link
Member

dmohns commented Nov 7, 2024

Hey @beesaferoot

there is some inconsistency in the Python library itself regarding the null values. More information here: EnAccess/OpenPAYGO-python#25

It's not really clear to me what the "correct" behaviour should be. I'd say let's leave your PR like it is for now. But maybe we come back to it later on, once we clarified to intended behaviour 👌

@dmohns
Copy link
Member

dmohns commented Nov 7, 2024

Regarding the test cases: I would prefer both repos (or all for that matter 😉 ) to use the exact same test cases file. I found JSONL to be more comfortable in Python. Is it a big trouble to use JSONL in JavaScript? Quick Google seem to suggest there might not be a super easy way to do it 😩

WDYT? I could also change the Python lib to use JSON instead.

@beesaferoot
Copy link
Collaborator Author

beesaferoot commented Nov 7, 2024

I could also change the Python lib to use JSON instead.

@dmohns I think Python also works pretty well with JSON, so this would be preferred.

@dmohns
Copy link
Member

dmohns commented Nov 7, 2024

Thanks 🙏

@dmohns dmohns merged commit 2c323ae into main Nov 7, 2024
8 checks passed
@dmohns dmohns deleted the test-tokens branch November 7, 2024 10:30
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.

[Feature Request]: Update Test Tokens to the latest generated from reference Python Implementation
2 participants