raphiniert-com / ra-data-postgrest

react admin client for postgREST
MIT License
110 stars 34 forks source link

[Doc] Fix Custom schema example #121

Closed slax57 closed 8 months ago

slax57 commented 9 months ago

I believe the code example in the 'Custom schema' section of the docs is wrong, as the schema prop expects a function (() => string) and not a string directly.

scheiblr commented 9 months ago

Thank you for your concerns.

The documentation here is flawless. A look at the code ( here and here) reveals that it is right. Also, the test cases show it (e.g. here).

The reason to use a function rather than a string is, that like that it is possible to include some function which retrieves the schema name from another storage (such as the localStorage) and allows this value to change.

I hope I was able to clarify the decision and point out, that the documentation is correct.

Kind regards,

scheiblr commented 9 months ago

closed.

slax57 commented 9 months ago

Thank you for your answer.

However, I'm sorry to insist, but I think you might have misunderstood my point. I'm not questioning the fact that it needs to be a function, on the contrary, I agree!

What I'm saying is that, if you try to apply the code example from the docs directly (i.e. pass schema: localStorage.getItem("schema") || "api"), you will see a TS error telling you that schema cannot be a string.

That's why I suggested to fix that part of the Readme.md, but not the part saying it needs to be a function (with which I fully agree!).

I hope these explanations will make my request clearer.

In any case, it's only a small error, and it's easy to figure out how to fix it in user-land, but still, thought I would open a PR to improve the docs.

scheiblr commented 8 months ago

Sorry, my mistake. Thanks for insisting :) I'm gonna accept the fix :) thank you very much!

coveralls commented 8 months ago

Pull Request Test Coverage Report for Build 7924150770

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Totals Coverage Status
Change from base Build 7359427530: 0.0%
Covered Lines: 265
Relevant Lines: 276

💛 - Coveralls