Skip to content

Add KeyActions enum #455

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jan 9, 2025
Merged

Add KeyActions enum #455

merged 6 commits into from
Jan 9, 2025

Conversation

MiaKoring
Copy link
Contributor

@MiaKoring MiaKoring commented Dec 9, 2024 •
edited
Loading

Pull Request

Related issue

Fixes #418

What does this PR do?

  • add KeyAction enum with all supported cases and .unknown(String)
  • extends KeyAction with an initializer with String
  • extends KeyAction with a String representation of the enum value
  • adds initializer with [KeyAction] instead of String for actions to KeyParams
  • extends Key&KeyParams with value “enumActions” containing the actions as KeyAction representation

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

@Sherlouk
Copy link
Collaborator

Sherlouk commented Dec 9, 2024

This is awesome, thanks! Will give it a full review shortly and look to get both merged in (in preparation for a release once a few more breaking changes are compiled).

Sherlouk
Sherlouk reviewed Dec 10, 2024
View reviewed changes
Tests/MeiliSearchIntegrationTests/KeysTests.swift
print(error.localizedDescription)
dump(error)
XCTFail("Failed to create a key")
keyExpectation.fulfill()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming this test is a "happy path", and as such we should remove this expectation fulfill, and the comment above which states "is expected to fail".

@MiaKoring
Breaking: replaced String value of KeyAction with Enum
3292a9a 
+ removed keyExpection and comment of KeyTest with unsupported action

tests passing
Sherlouk
Sherlouk previously approved these changes Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@Sherlouk Sherlouk left a comment

Choose a reason for hiding this comment

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

Will need to merge the other PR first @curquiza, but both should be good to go.

Let's not do a release yet though -- I'll work on some other tweaks soon as my cal frees up!

@Sherlouk Sherlouk added the breaking-change The related changes are breaking for the users label Jan 4, 2025
@curquiza
Copy link
Member

curquiza commented Jan 7, 2025

@MiaKoring
can you fix the git conflict please? we had to merge #459 first, sorry

@Sherlouk
Copy link
Collaborator

Sherlouk commented Jan 7, 2025

Sorry @MiaKoring! With this PR we'll do both the MTask and KeyActions.

CI will be working once you bring in latest changes, and so can get it merged.

Thanks

@MiaKoring
Merge branch 'main' into makeKeyActionsEnum
dfe5625 
# Conflicts:
#	Tests/MeiliSearchIntegrationTests/Utils.swift
@MiaKoring
Merged meilisearch#461
7f345ee
@MiaKoring
Copy link
Contributor Author

Tests are passing, if there is something else that needs to be done, let me know

Sherlouk
Sherlouk approved these changes Jan 7, 2025
View reviewed changes
Copy link
Collaborator

@Sherlouk Sherlouk left a comment

Choose a reason for hiding this comment

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

All looking good. Ignore the linter error, I'll tweak SwiftLint in a separate PR as it's a little sensitive right now.

@curquiza
Copy link
Member

curquiza commented Jan 7, 2025

@Sherlouk I cannot merge with the linter error. Do you want to open a PR to remove the linting issue? we will merge this PR after

@Sherlouk Sherlouk mentioned this pull request Jan 7, 2025
Closed
curquiza
curquiza approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

bors merge

@meili-bors meili-bors
Copy link
Contributor

meili-bors bot commented Jan 9, 2025

Build succeeded:

@meili-bors meili-bors bot merged commit e4cb549 into meilisearch:main Jan 9, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sherlouk Sherlouk Sherlouk approved these changes

@curquiza curquiza curquiza approved these changes

Assignees
No one assigned
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Key Actions Enum
3 participants
@MiaKoring @Sherlouk @curquiza