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

[Question] When NRT is added, should "Key(int index)" change as well? #209

Open
datvm opened this issue Apr 23, 2023 · 0 comments
Open

[Question] When NRT is added, should "Key(int index)" change as well? #209

datvm opened this issue Apr 23, 2023 · 0 comments
Labels
Question Question about this project

Comments

@datvm
Copy link

datvm commented Apr 23, 2023

This is a follow up of #205 . While making the changes of adding NRT to the project, I found an interesting change we may want to make as well: the KeyAsync(int index) method and its related functions. Right now it returns string? (null if the index is out of range). Should we:

  • Keep it as it is right now (Javascript convention, a null is returned if the index is invalid).
    • Pros: Consistent with Javascript code. The result is returned as-is. No change is needed and no breaking change is introduced.
    • Cons: Developers are ended up with string? result which most of the time should not be null and a ! or ? operator or null check is needed.
  • We throw IndexOutOfRangeException like most C# methods if the index is out of bound.
    • Pros: Consistent with C# convention. Most of the time developers do not have to worry about a null value if their code is sound.
    • Cons: Extra null or bound check for every function call. This is a breaking change.
@datvm datvm added Question Question about this project Triage Issue needs to be triaged labels Apr 23, 2023
@chrissainty chrissainty removed the Triage Issue needs to be triaged label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question Question about this project
Projects
None yet
Development

No branches or pull requests

2 participants