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

Add support for tags #10

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

playfulkittykat
Copy link
Contributor

@playfulkittykat playfulkittykat commented Oct 20, 2024

Here's my attempt at adding support for tags. It isn't entirely complete yet, but I wanted to get your opinion before moving forward.

I've gone with a builder pattern for tags::Query, but differently than PoolSearch. If you like it, I can update everything to the same style.

Outstanding Tasks:

  • verify Cursor logic
  • mockito tests
  • better documentation for tags::Query (and everything else.)
  • more examples

@nasso
Copy link
Owner

nasso commented Oct 20, 2024

thank you!

i like the Cursor and the Query API seems nice. we can work on making it more consistent with PoolSearch in a separate PR to make things easier

i am not familiar with the tag search API (it didn't exist back then) so i dont know much about the a1234 and b1234 bits

@playfulkittykat
Copy link
Contributor Author

i like the Cursor and the Query API seems nice. we can work on making it more consistent with PoolSearch in a separate PR to make things easier

Thank you! That works for me

i am not familiar with the tag search API (it didn't exist back then) so i dont know much about the a1234 and b1234 bits

It looks like it uses the same code path as the posts endpoint, so the a... and b... should be similar to:

rs621/src/post.rs

Lines 388 to 392 in 236bac4

match this.next_page {
SearchPage::Page(i) => format!("{}", i),
SearchPage::BeforePost(i) => format!("b{}", i),
SearchPage::AfterPost(i) => format!("a{}", i),
},

@playfulkittykat
Copy link
Contributor Author

I'm happy with where this is now. Let me know what you think!

Copy link
Owner

@nasso nasso left a comment

Choose a reason for hiding this comment

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

pretty cool! looking mostly good to me, just minor nitpicks (also minor conflicts)

regarding the mocked data (in tests and json files), i'd prefer using SFW data, please 😅

src/tag.rs Outdated Show resolved Hide resolved
src/tag.rs Outdated Show resolved Hide resolved
src/tag.rs Outdated Show resolved Hide resolved
src/client.rs Outdated
Comment on lines 93 to 95
Self::Page(p) => write!(f, "{}", p),
Self::Before(p) => write!(f, "b{}", p),
Self::After(p) => write!(f, "a{}", p),
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Self::Page(p) => write!(f, "{}", p),
Self::Before(p) => write!(f, "b{}", p),
Self::After(p) => write!(f, "a{}", p),
Self::Page(p) => write!(f, "{p}"),
Self::Before(p) => write!(f, "b{p}"),
Self::After(p) => write!(f, "a{p}"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I also add rust-version = "1.58" to Cargo.toml?

Copy link
Owner

Choose a reason for hiding this comment

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

oh maybe, i didn't check what the msrv is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it'd be around 1.70 because of some dependencies.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah that seems okay to be honest?

src/post.rs Outdated Show resolved Hide resolved
@playfulkittykat
Copy link
Contributor Author

playfulkittykat commented Nov 6, 2024

I've added Cargo.lock to the repository (see here for the general reasoning) specifically because I've added 1.70 to GitHub Actions, and if a dependency changes its rust-version, that would cause CI to randomly break.

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.

2 participants