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

[scr_ecto-103] Improve options/configuration described in README #104

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

Conversation

theirishpenguin
Copy link

The Problem

The README does not mention options such as :allow_overflow_page_number and direct configuration such as :max_page_size. This is noted in issue #103

This PR is to (make a start at least) on correcting that.

@theirishpenguin
Copy link
Author

One check fails above with the following error...

image

Is there something I should do to correct this?

(As this is just a documentation change, I'm conscious it's probably best if I don't go messing with the actual codebase as part of this PR 😄 )

@theirishpenguin
Copy link
Author

A fellow Elixir-er kindly helped me out with the build issue at https://github.com/theirishpenguin/scrivener_ecto/pull/1/files

Looking at that PR, it seems like there is an issue building scrivener_ecto with elixir: '1.15.0' / otp: '26.0.1'. I guess that this means that an update will be needed for scrivener_ecto to keep it building on otp: '26.0.1'. I might open an issue on this repo to call this out explicitly. (Given the Low Maintenance warning on this repo, I guess you won't have time to work on this actively @drewolson - which is understandable).

@cpjolicoeur
Copy link
Member

@theirishpenguin We have updated the master branch of Scrivener_Ecto to support OTP 26. If you want to rebase your PR work here off the current master, we can look at getting these documentation changes in.

@theirishpenguin theirishpenguin force-pushed the scr_ecto-103--improve-docs branch from 4947391 to 2bde3cf Compare September 9, 2024 19:37
@theirishpenguin theirishpenguin force-pushed the scr_ecto-103--improve-docs branch from 2bde3cf to b99cc00 Compare September 9, 2024 19:39
@theirishpenguin
Copy link
Author

@cpjolicoeur Thanks 🙌

I've just rebased this PR now off the current master.

@cpjolicoeur cpjolicoeur changed the base branch from master to main October 14, 2024 13:22
@theirishpenguin theirishpenguin force-pushed the scr_ecto-103--improve-docs branch 3 times, most recently from abdc4ae to b99cc00 Compare November 13, 2024 18:39
@theirishpenguin
Copy link
Author

I have tried to kick off the build again with a "reinitialise checks" commit, but it seems like that needs approval from a maintainer.

(I did have a few problems with that commit - all fixed now - so maybe that is why it requires manual approval).

Is it possible for someone to kick off a build on this PR?

@theirishpenguin
Copy link
Author

Seems like all checks have been passed 🥳

image

Can this now be merged?


Here is an explanation of the supported options...

* `:allow_overflow_page_number` - If set to true then when you supply a page greater than the total pages, it will return an empty list for entries. If set to false then it will (silently) treat any supplied `page` arg as if it was equal to the total pages. Default is false.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 71 you mention "there are several supported options", but then we proceed to only document a single option. We should either a) document the options available that scrivener_ecto uses, or b) point people (via link) to the documented options directly in the Scrivener docs.


* `:allow_overflow_page_number` - If set to true then when you supply a page greater than the total pages, it will return an empty list for entries. If set to false then it will (silently) treat any supplied `page` arg as if it was equal to the total pages. Default is false.

## Other Configuration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two sections (Supported Options and Other Configuration) can just be a single "Scrivener configuration" section as they both directly related to configuring Scrivener directly. There is no need to separate them out into different sections.

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