search

KenKundert / nestedtext_tests

5 stars 2 forks source link

Add Ruby API and test case #7

Closed erikw closed 2 years ago

erikw commented 2 years ago

As per the encouragement, I'm sharing back the Ruby API that I added during development of nestedtext-ruby.

Thanks for creating this repo, it was very helpful during development to check edge cases that I missed in my own testing!

erikw commented 2 years ago

@KenKundert friendly ping

(as I saw you mentioned in https://github.com/KenKundert/nestedtext_tests/issues/5#issuecomment-1029565924 that you don't get notifications from this repo, so maybe this PR was missed as well 😃 )

KenKundert commented 2 years ago

Sorry about the delayed response. I would be uncomfortable adding this code into the official repository, as I have no way to maintain it. How about a pointer to your repository from the official tests?

Why did you override the expected error for test_cases/dict_22? That does not seem right.

erikw commented 2 years ago

I would be uncomfortable adding this code into the official repository, as I have no way to maintain it. How about a pointer to your repository from the official tests?

@KenKundert Sure, I'm fine with this as well. I updated to PR to move the Ruby API out of this repo to nestedtext-ruby instead. I added a link from the README instead as you suggested. Feel free to update the PR by pushing new commits to it, in case you'd like to re-formulate.

Why did you override the expected error for test_cases/dict_22? That does not seem right.

Hmm I'm not sure that you looked at the right diff here 🤔? Unless I misunderstand you, dict_22/dump_err.json is not changed in this PR.

I added the dict_22/dump_in.rb that is analogous to dict_22/dump_in.py, so that Ruby users of the official tests also can test the case of dict_22 😃 .

KenKundert commented 2 years ago

Sorry. As you surmised, I misunderstood the purpose of dict_22/dump_in.rb. I also apologize for the mixed-messages on submitting your test-API. That was due to a small difference in perspective between the authors.

Just so you know, we plan to refactor the official tests to make them more flexible and more supportable. At that point we will try to reduce the need for these kinds of work-arounds.

erikw commented 2 years ago

Alright no problems.

@KenKundert Did the refactor already begin? I noticed that directly after merging this PR, dict_22/dump_in.rb was actually removed again in https://github.com/KenKundert/nestedtext_tests/commit/57f736717864478b473371029b56ef53ce18ce3c 🤔.

This means I can't switch to the master branch of this repo from my fork yet as dict_22 still is for Python only.

I have some feedback gathered after working with the official tests that I can share with you for the upcoming refactor! I'll open a separate issue.

KenKundert commented 2 years ago

We seem to be misunderstanding each other. I think that is my fault. My expectation was that there would only be the comment in the Readme because I would not be able to support your Ruby code long term. So I deleted your code. In the short term I have no problem adding it back in if desired, with the expectation that it will be removed during the refactor.

erikw commented 2 years ago

Yes I realize now that we misunderstood each other. For me, the Ruby API (official_tests_api.rb) is separate from the Ruby test case (dict_22/dump_in.rb).

I think there's a point of keeping the test case still, as a clear reminder for the upcoming refactoring that supporting more complicated types in different languages is a thing to address. But if this is already a clear TODO in the refactoring, then all good.

Your choice :)

KenKundert commented 2 years ago

Yes, we won't forget. Let's keep things the way they are for now.