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

test: remove undocumented and undefined usage of req.reject #186

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

schwma
Copy link
Member

@schwma schwma commented Oct 7, 2024

fixes issue mentioned in #185

(closes #185)

@schwma schwma requested a review from a team as a code owner October 7, 2024 08:26
@schwma schwma changed the title chore: fix i18n test that doesn't fill placeholders test: fix i18n test that doesn't fill placeholders Oct 7, 2024
@schwma schwma enabled auto-merge (squash) October 7, 2024 08:31
@danjoa
Copy link
Contributor

danjoa commented Oct 7, 2024

No, this also does not address the intent: We need to clarify the expected API behaviour instead:
What do we expect when we call an i18n.at(key,locale?,args?) function with only key, or key and locale, but no args?

  • Option 1: the text from the bundle as is, i.e., without args applied?
  • Option 2: the text from the bundle with all args filled with 'NULL'?

As option 2 has little value if any at all, I clearly vote for Option 1.
And as we haven't clarified that in @sap/cds, and your graphql tests should not have any expectations in that regard (why should it?), let's just remove this test please.

@schwma schwma disabled auto-merge October 7, 2024 17:32
@schwma
Copy link
Member Author

schwma commented Oct 7, 2024

I see, thanks for the clarification! I will remove the test 👍

@schwma schwma changed the title test: fix i18n test that doesn't fill placeholders test: remove undocumented and undefined usage of req.reject Oct 9, 2024
@schwma schwma enabled auto-merge (squash) October 9, 2024 14:17
@schwma schwma merged commit a204b12 into main Oct 9, 2024
5 checks passed
@schwma schwma deleted the i18n-test-without-args branch October 9, 2024 15:32
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.

3 participants