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

Support for additional fields in Tanks and filter in retrieve #1045

Merged
merged 10 commits into from
Sep 1, 2024

Conversation

EfronC
Copy link
Contributor

@EfronC EfronC commented Aug 2, 2024

[POSSIBLY INCOMPLETE]

This is a small PR just to show the idea of the initial added support for extra fields in Tanks. The commit adds a new file called Constants.pm in utils, since I wanted to have a variable with the extra fields separated of the Tank code to keep the structure defined and easy to modify, and because I saw Generic.pm was full with functions, I decided to create a file exclusively for variables, otherwise both work the same.

This PR also adds support in the "Get All Tankoubons" EP to send a param name, so you can filter the result, still I'm not sure if ti should be done this way, since if someone ever gets to have so many Tanks(Which I doubt, but is possible), this might take some time to do the search, since is visiting the whole list twice at worst before paginating.

This probably has a few issues, so I keep pending for comments about changes that can be done to improve this idea.

@EfronC
Copy link
Contributor Author

EfronC commented Aug 3, 2024

There is a bug that I just noted, where the get tankoubon EP appends an extra field "5":0 in each tank, but I've no idea why this is happening, I've tracked that it might be related to how the Tanks are created, since reducing the number of flag fields reduce the number in it, but still I've no idea where is that field being inserted.

Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

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

Needs additional tests imo, but this makes sense to me.
A few general code quality comments and Qs. 👍

lib/LANraragi/Controller/Api/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Utils/Constants.pm Outdated Show resolved Hide resolved
lib/LANraragi/Utils/Constants.pm Outdated Show resolved Hide resolved
lib/LANraragi/Utils/Routing.pm Outdated Show resolved Hide resolved
Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

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

I'll have to give fetch_data a more indepth think, but did find some other stuff in the meantime.

lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

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

This looks markedly better to me. 👍 Just a bit more cleanup I've noticed.

tools/Documentation/api-documentation/tankoubon-api.md Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
Copy link

holopin-bot bot commented Sep 1, 2024

Congratulations @EfronC, the maintainer of this repository has issued you a holobyte! Here it is: https://holopin.io/holobyte/cm0jrwd9010420cl6oicokw6l

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@Difegue Difegue merged commit b49bf52 into Difegue:dev Sep 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants