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

Fix examples & enums #778

Conversation

dirtycajunrice
Copy link
Contributor

@dirtycajunrice dirtycajunrice commented Nov 21, 2024

Changes

How this PR will be tested

  • Open the dashboard and click X. Result: A modal should appear.
  • Call the /foo/bar API. Result: Returns 200 with "baz" in the response body.

Output

(Example: Screenshot/GIF for UI changes, cURL output for API changes)


PR-Codex overview

This PR primarily focuses on code cleanup and refactoring, improving error handling, updating comments, and enhancing type definitions across various services and routes. It also includes modifications to schemas and responses to ensure better clarity and consistency.

Detailed summary

  • Added .idea and JetBrains-related files to .gitignore.
  • Improved error handling by removing specific error variables in catch blocks.
  • Changed let declarations to const where applicable for better immutability.
  • Updated response schemas to use more precise types and structures.
  • Modified example responses to reflect more accurate data types.
  • Enhanced documentation comments for clarity and completeness.
  • Refactored service methods to use more descriptive names (e.g., getAll() to list()).
  • Added new testWebhook method in WebhooksService for testing webhook functionality.
  • Updated package versions for ESLint and TypeScript-related packages.

The following files were skipped due to too many changes: sdk/src/services/ContractMetadataService.ts, sdk/src/services/ContractService.ts, src/tests/auth.test.ts, sdk/src/services/ContractSubscriptionsService.ts, sdk/src/services/AccountFactoryService.ts, sdk/src/services/ContractRolesService.ts, sdk/src/services/ContractRoyaltiesService.ts, sdk/src/services/ConfigurationService.ts, sdk/src/services/AccountService.ts, sdk/src/services/TransactionService.ts, sdk/src/services/MarketplaceOffersService.ts, sdk/src/services/MarketplaceEnglishAuctionsService.ts, yarn.lock, sdk/src/services/BackendWalletService.ts, sdk/src/services/MarketplaceDirectListingsService.ts, sdk/src/services/DeployService.ts, sdk/src/services/Erc20Service.ts, sdk/src/services/Erc721Service.ts, sdk/src/services/Erc1155Service.ts, sdk/old_openapi.json

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@dirtycajunrice dirtycajunrice changed the title Fix direct-listings/get-all example and add descriptions to status enum Fix examples & enums Nov 21, 2024
@dirtycajunrice dirtycajunrice marked this pull request as ready for review November 21, 2024 22:37
Copy link

socket-security bot commented Nov 22, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@eslint-community/[email protected] None 0 378 kB eslint-community-bot, michaeldeboey
npm/@eslint-community/[email protected] None 0 473 kB eslint-community-bot, michaeldeboey
npm/@eslint/[email protected] None 0 314 kB eslintbot
npm/@eslint/[email protected] None 0 60 kB eslintbot
npm/@eslint/[email protected] None +2 871 kB eslintbot, openjsfoundation
npm/@eslint/[email protected] None 0 14.9 kB eslintbot, openjsfoundation
npm/@eslint/[email protected] None 0 55.5 kB eslintbot
npm/@eslint/[email protected] None +2 123 kB eslintbot, openjsfoundation
npm/@humanfs/[email protected] None 0 72.7 kB nzakas
npm/@humanfs/[email protected] None +1 88.4 kB nzakas
npm/@humanwhocodes/[email protected] None 0 64.9 kB nzakas
npm/@typescript-eslint/[email protected] None 0 2.73 MB bradzacher, jameshenry
npm/@typescript-eslint/[email protected] None 0 18.8 kB bradzacher, jameshenry
npm/@typescript-eslint/[email protected] None 0 607 kB bradzacher, jameshenry
npm/@typescript-eslint/[email protected] None 0 122 kB bradzacher, jameshenry
npm/@typescript-eslint/[email protected] None 0 173 kB bradzacher, jameshenry
npm/@typescript-eslint/[email protected] Transitive: environment +1 1.02 MB jameshenry
npm/@typescript-eslint/[email protected] None 0 286 kB bradzacher, jameshenry
npm/@typescript-eslint/[email protected] None +1 55.4 kB bradzacher, jameshenry
npm/[email protected] None 0 20.8 kB lydell
npm/[email protected] None 0 152 kB eslintbot
npm/[email protected] environment Transitive: eval, filesystem, shell +11 5.05 MB eslintbot
npm/[email protected] None 0 1.04 MB michaelficarra
npm/[email protected] None 0 1.19 MB joshuakgoldberg

🚮 Removed packages: npm/@eslint-community/[email protected], npm/@eslint-community/[email protected], npm/@eslint/[email protected], npm/@eslint/[email protected], npm/@humanwhocodes/[email protected], npm/@humanwhocodes/[email protected], npm/@types/[email protected], npm/@typescript-eslint/[email protected], npm/@typescript-eslint/[email protected], npm/@typescript-eslint/[email protected], npm/@typescript-eslint/[email protected], npm/@typescript-eslint/[email protected], npm/@typescript-eslint/[email protected], npm/@typescript-eslint/[email protected], npm/@typescript-eslint/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

sdk/src/core/request.ts Fixed Show fixed Hide fixed
Copy link
Member

@d4mr d4mr left a comment

Choose a reason for hiding this comment

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

would need to make a pass, remove es-lint related stuff and ignore autogen sdk code for formatter to get a more usable diff

Copy link
Member

Choose a reason for hiding this comment

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

we use biomejs as standard and had removed eslint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didnt add anything, i just migrated the existing config that already existed to one that didnt break.

Copy link
Member

Choose a reason for hiding this comment

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

I think your formatter run on the autogen code as well, making the code diff hard to see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not auto formatting anything in the SDK. This is what happens if you pull the repo, and run generate:sdk. No changes.

});
it("with leading wildcard", () => {
expect(sanitizeOrigin("*.foobar.com")).toEqual(RegExp(".*.foobar.com"));
expect(sanitizeOrigin("*.foobar.com")).toEqual(/.*.foobar.com/);

Check failure

Code scanning / CodeQL

Incomplete regular expression for hostnames High test

This regular expression has an unescaped '.' before 'foobar.com', so it might match more hosts than expected.

Copilot Autofix AI about 1 month ago

To fix the problem, we need to escape the . character in the regular expression to ensure it matches only literal dots and not any character. This can be done by replacing .*.foobar.com with .*\.foobar\.com. This change ensures that the regex will only match subdomains of foobar.com and not any other unintended domains.

Suggested changeset 1
src/tests/cors.test.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/tests/cors.test.ts b/src/tests/cors.test.ts
--- a/src/tests/cors.test.ts
+++ b/src/tests/cors.test.ts
@@ -8,3 +8,3 @@
   it("with leading wildcard", () => {
-    expect(sanitizeOrigin("*.foobar.com")).toEqual(/.*.foobar.com/);
+    expect(sanitizeOrigin("*.foobar.com")).toEqual(/.*\.foobar\.com/);
   });
EOF
@@ -8,3 +8,3 @@
it("with leading wildcard", () => {
expect(sanitizeOrigin("*.foobar.com")).toEqual(/.*.foobar.com/);
expect(sanitizeOrigin("*.foobar.com")).toEqual(/.*\.foobar\.com/);
});
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link

github-actions bot commented Dec 4, 2024

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this PR will be closed in 3 days.

@arcoraven
Copy link
Contributor

I'm pulling pieces from this PR and merging them into main. I won't reopen this unless you want to rebase it (probably more effort than it's worth).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants