-
Notifications
You must be signed in to change notification settings - Fork 3
support prefix based deletion #8
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 23f9500 in 33 seconds
More details
- Looked at
71lines of code in2files - Skipped
1files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_lF0AvzLw14FAasde
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 3 days left in your free trial, upgrade for $20/seat/month or contact us.
23f9500 to
c61f5e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on c61f5e3 in 21 seconds
More details
- Looked at
71lines of code in2files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/main.ts:6
- Draft comment:
Whenprefixis true,keycan be empty, butgetInputis set torequired: trueforkey. Consider settingrequired: falseand handling the empty case separately. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/main.ts:19
- Draft comment:
The URL modification for prefix-based deletion is correct and aligns with the PR description. - Reason this comment was not posted:
Confidence changes required:0%
The code correctly appends?prefixto the URL whenprefixis true, which aligns with the PR description.
Workflow ID: wflow_6MQUMDhl2xwapTMe
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 3 days left in your free trial, upgrade for $20/seat/month or contact us.
c61f5e3 to
706b7f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on 706b7f0 in 56 seconds
More details
- Looked at
374lines of code in5files - Skipped
2files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. src/main.ts:23
- Draft comment:
EnsurecacheTokenis defined before making the fetch request to avoid unauthorized errors. - Reason this comment was not posted:
Comment did not seem useful.
2. src/main.ts:22
- Draft comment:
EnsurerepoNameis defined before making the fetch request to avoid sending incorrect headers. - Reason this comment was not posted:
Comment did not seem useful.
3. src/main.ts:18
- Draft comment:
EnsurebaseUrlis defined before making the fetch request to avoid incorrect URL formation. - Reason this comment was not posted:
Comment did not seem useful.
4. src/main.ts:24
- Draft comment:
Ensureregionis defined before making the fetch request to avoid sending incorrect headers. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_JWEqP9qGhpkPdDRW
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 3 days left in your free trial, upgrade for $20/seat/month or contact us.
src/main.ts
Outdated
|
|
||
| async function run(): Promise<void> { | ||
| try { | ||
| const cacheKey = getInput("key", { required: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getInput call for key is marked as required, but the logic allows for an empty key when prefix is true. Consider removing the required: true option to align with the logic.
| const cacheKey = getInput("key", { required: true }); | |
| const cacheKey = getInput("key"); |
706b7f0 to
5e51f74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 5e51f74 in 19 seconds
More details
- Looked at
382lines of code in5files - Skipped
2files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. jest.config.js:14
- Draft comment:
The Jest configuration looks good and follows best practices for TypeScript projects. - Reason this comment was not posted:
Confidence changes required:0%
The jest.config.js file is correctly set up for TypeScript testing with Jest. The configuration is clear and follows best practices.
2. package.json:28
- Draft comment:
The package.json is well-configured with necessary dependencies and scripts for building, formatting, linting, and testing. - Reason this comment was not posted:
Confidence changes required:0%
The package.json file correctly includes the necessary dependencies and devDependencies for the project, including Jest and TypeScript support.
3. src/__tests__/main.test.ts:163
- Draft comment:
The test cases are comprehensive and cover various scenarios for the deleteCache function, including success, 404, and error cases. - Reason this comment was not posted:
Confidence changes required:0%
The test file is well-structured and covers various scenarios for the deleteCache function, including success, 404, and error cases.
Workflow ID: wflow_zvyp5vfBqBzkYpf8
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 3 days left in your free trial, upgrade for $20/seat/month or contact us.
5e51f74 to
51cfa0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 51cfa0c in 21 seconds
More details
- Looked at
383lines of code in5files - Skipped
2files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/__tests__/main.test.ts:13
- Draft comment:
Ensure thatjest.mocked(fetch)is correctly typed for better type safety. This is important for maintaining type integrity in tests. - Reason this comment was not posted:
Confidence changes required:20%
The test file is correctly structured, but the mock fornode-fetchshould ensure that thefetchfunction is properly typed. The current usage ofjest.mocked(fetch)is correct, but it's good to ensure that the mock is typed correctly for better type safety.
2. src/main.ts:23
- Draft comment:
Consider adding a check to ensurecacheTokenis not undefined before making the fetch request to prevent potential runtime errors. - Reason this comment was not posted:
Confidence changes required:30%
ThedeleteCachefunction inmain.tsis well-structured, but it could benefit from additional validation for thecacheTokento ensure it's not undefined before making the fetch request. This would prevent potential runtime errors.
Workflow ID: wflow_Ro6akheyjmBQndFy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 3 days left in your free trial, upgrade for $20/seat/month or contact us.
Important
Add prefix-based cache deletion support with updated documentation and tests.
prefixinput insrc/main.tsfor prefix-based cache deletion.keyto be empty whenprefixis true to delete all caches.?prefixwhenprefixis set.README.mdto includeprefixinput in inputs table.jest.config.jsfor test configuration.src/__tests__/main.test.tsfor new prefix behavior and error handling.jest,@types/jest, andts-jesttopackage.json.This description was created by
for 51cfa0c. It will automatically update as commits are pushed.