(Windows) build issues and contributing docs #1458
Conversation
WalkthroughWalkthroughThe recent updates bring significant enhancements to the project. Notably, improvements include the addition of a Changes
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
Actions PerformedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (9)
packages/ide/jetbrains/package.jsonis excluded by!**/*.jsonpackages/language/package.jsonis excluded by!**/*.jsonpackages/plugins/trpc/package.jsonis excluded by!**/*.jsonpackages/runtime/package.jsonis excluded by!**/*.jsonpackages/schema/package.jsonis excluded by!**/*.jsonpackages/sdk/package.jsonis excluded by!**/*.jsonpackages/server/package.jsonis excluded by!**/*.jsonpackages/testtools/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!pnpm-lock.yaml,!**/*.yaml
Files selected for processing (4)
- .gitattributes (1 hunks)
- CONTRIBUTING.md (1 hunks)
- packages/testtools/src/db.ts (1 hunks)
- packages/testtools/src/schema.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitattributes
Additional Context Used
LanguageTool (7)
CONTRIBUTING.md (7)
Near line 12: Loose punctuation mark.
Context: ...stcommands. -ZENSTACK_TEST_DB_USER`: The postgres username, for a user with ...
Near line 13: Loose punctuation mark.
Context: ...t:postgres. -ZENSTACK_TEST_DB_PASS: Password for said user. Default: `abc12...
Near line 14: Loose punctuation mark.
Context: ...ult:abc123. -ZENSTACK_TEST_DB_NAME: Default database to connect onto. This ...
Near line 15: Loose punctuation mark.
Context: ...t:postgres. -ZENSTACK_TEST_DB_HOST: Hostname or IP to connect onto. Default...
Near line 16: Loose punctuation mark.
Context: ...:localhost. -ZENSTACK_TEST_DB_PORT: Port number to connect onto. Default: `...
Near line 68: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...# Development workflow ZenStack adopts a very simple development workflow: 1. Changes shou...
Near line 98: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ... routes in a framework-independent way. Currently supports "rpc" and "rest" styles. 1. F...
Additional comments not posted (5)
packages/testtools/src/db.ts (2)
3-7: Environment variable usage enhances configuration flexibility and security.
9-17: Centralizing database connection logic inconnect()enhances maintainability and reusability.CONTRIBUTING.md (2)
8-16: Updated prerequisites section accurately specifies the requiredpnpmversion and details for setting up a PostgreSQL database.
20-25: Clear instructions for configuring Git to handle symlinks on Windows.packages/testtools/src/schema.ts (1)
175-175: Path normalization inloadSchemaensures compatibility across different operating systems.
|
As for the local publishing workflow, the suggestion in
What I ended up doing was to first bump the package.json versions locally, and then publish with that: This way, I could then install the prerelease version in my actual project and see my changes. |
…t usage of symlinks, which requires extra steps on Windows.
Running `pnpm build` on windows builds generated files with CRLF line endings. Adding the `eol=lf` entry in the `.gitattributes` file enforces LF line endings when files are staged, so line ending changes on those generated files are negated / ignored. Furthermore, this ensures consistency on newly added files.
…ripts; and remove single quotes when we don't need them. This improves windows compatibility.
…nfiguring connection details through environment variables to allow slightly more flexibility.
WimTibackx
force-pushed
the
contributing-docs
branch
from
51b8303 to
67eeeeb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
CONTRIBUTING.md (1)
Line range hint
68-68: Consider using a less subjective description than "very simple" for the development workflow to maintain professional tone.- ZenStack adopts a very simple development workflow: + ZenStack adopts a straightforward development workflow:
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (9)
packages/ide/jetbrains/package.jsonis excluded by!**/*.jsonpackages/language/package.jsonis excluded by!**/*.jsonpackages/plugins/trpc/package.jsonis excluded by!**/*.jsonpackages/runtime/package.jsonis excluded by!**/*.jsonpackages/schema/package.jsonis excluded by!**/*.jsonpackages/sdk/package.jsonis excluded by!**/*.jsonpackages/server/package.jsonis excluded by!**/*.jsonpackages/testtools/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!pnpm-lock.yaml,!**/*.yaml
Files selected for processing (3)
- .gitattributes (1 hunks)
- CONTRIBUTING.md (1 hunks)
- packages/testtools/src/db.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .gitattributes
Additional Context Used
LanguageTool (7)
CONTRIBUTING.md (7)
Near line 12: Loose punctuation mark.
Context: ...stcommands. -ZENSTACK_TEST_DB_USER`: The postgres username, for a user with ...
Near line 13: Loose punctuation mark.
Context: ...t:postgres. -ZENSTACK_TEST_DB_PASS: Password for said user. Default: `abc12...
Near line 14: Loose punctuation mark.
Context: ...ult:abc123. -ZENSTACK_TEST_DB_NAME: Default database to connect onto. This ...
Near line 15: Loose punctuation mark.
Context: ...t:postgres. -ZENSTACK_TEST_DB_HOST: Hostname or IP to connect onto. Default...
Near line 16: Loose punctuation mark.
Context: ...:localhost. -ZENSTACK_TEST_DB_PORT: Port number to connect onto. Default: `...
Near line 68: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...# Development workflow ZenStack adopts a very simple development workflow: 1. Changes shou...
Near line 98: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ... routes in a framework-independent way. Currently supports "rpc" and "rest" styles. 1. F...
Markdownlint (1)
CONTRIBUTING.md (1)
120: null
Bare URL used
Biome (1)
packages/testtools/src/db.ts (1)
7-7: Use Number.parseInt instead of the equivalent global.
Additional comments not posted (6)
packages/testtools/src/db.ts (4)
3-7: The use of environment variables for database configuration enhances flexibility and aligns with best practices.
9-17: Centralizing database connection settings in theconnectfunction is a clean and maintainable approach.
20-24: Proper management of the database connection pool increatePostgresDbfunction, including appropriate cleanup, is well implemented.
28-30: Good practice in managing the database connection pool indropPostgresDbfunction, ensuring resources are properly released.CONTRIBUTING.md (2)
8-16: Updating the prerequisites with specific version requirements and detailed setup instructions for PostgreSQL enhances clarity for new contributors.
20-25: Clear instructions for setting up symlinks on Windows, including a useful external reference, will aid Windows users in setting up their development environment correctly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
I noticed some Windows-specific build failures and some gaps in the
CONTRIBUTING.mddocs.I have the PR currently in draft mode because I am still doing some more checks on the tests, and I have yet to check the publishing. Felt it was sufficient to start opening it up to review, though.
I'd be happy to provide more details as needed.
This PR addresses the following:
I haven't verified yet whether PNPM 9.x works, but I figured it might be useful to specify the version reflected by the lockFileVersion. This part isn't really Windows specific, I hadn't really caught on to the Windows-specificness of my issues yet at this point :) .
core.symlinksshould be trueThe symlinks in use in the repo don't work out of the box on Windows, but require a bit of Windows settings fiddling and git settings fiddling.
This avoids Windows users executing
pnpm buildor adding new files and thereby introducing inconsistent line endings. Current settings in.gitattributesindicate that everything but.batfiles use LF. I'm not sure whether the.batfile specifically has to be CRLF, but given that it's currently the case I assumed it would have to be.I ran
git add --update --normalizeto verify line endings, but all current files match this configuration.Single quotes are a bit of an issue, unfortunately.
In glob-style arguments I replaced the single quotes with escaped double quotes - which are already in use in some glob-style arguments.
For the pack-destination param I removed the quotes, as some package.jsons already have that param without quotes - so I presumed it to be safe.
gradlewscript for the current OSI added a dependency on
run-script-osfor this. This seems like a fairly sensible and lightweight way to handle it.Non-Windows specific: document that a postgres database is used in some tests and should be available for contributors executing tests.
Non-Windows specific: introduce environment variables to override default postgres connection details in integration tests.
In my case, I wanted to use a user other than the default
postgresuser, so I could have a bit more fine-grained control to the integration tests user. And honestly, so I wouldn't inadvertedly change the password to my root user and break my tests setup.This would also allow development on a local system, with postgres running within docker or setups like that.
Added some path normalization, because current processing was resulting in incorrect paths.