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

Fix: Error messages now respect custom option names and values. #24

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jmetrikat
Copy link

Previously, error messages in validatePage and validatePageSize didn’t make use of the names and values from custom options: opts.PageText, opts.SizeText, p.opts.MinPageSize and p.opts.MaxPageSize.

Now, they correctly substitute those values, and I’ve expanded the test cases to ensure proper behavior in error messages.

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c7283b2) to head (803f032).

Additional details and impacted files
@@            Coverage Diff            @@
##            master       #24   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           87        87           
=========================================
  Hits            87        87           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@webstradev
Copy link
Owner

Thanks for this, good suggestion, the only thing I don't love is directly checking errors strings rather than comparing them with errors is /errors as, but maybe in this case it's okay

@webstradev
Copy link
Owner

webstradev commented Feb 26, 2025

@jmetrikat I think the idea is good, but I would rather see it implemented with specific errors, that we than test for instead of just relying on string matching. Is that something you are willing to give a go? Otherwise I can take it over if you don't have the bandwidth.

Would be something like this:
image

And then there can just be separate tests for those validator functions to see if they error in the correct cases and with the correct values.

Seems a cleaner to me then testing a string in the json response.

@webstradev webstradev changed the base branch from master to main February 26, 2025 22:26
@jmetrikat
Copy link
Author

@webstradev looks good! considered a similar approach, but hesitated due to the larger changes needed. i don't have the capacity currently, so feel free to take it over!

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