fix(ci): disable line length rule for platform packages #8
Conversation
- Remove 'publish_to: none' from all packages - Change path dependencies to hosted dependencies (^2.0.0) - Remove 'resolution: workspace' for pub.dev compatibility - Ready for federated plugin publishing
- Restore 'resolution: workspace' for all packages - Restore path dependencies for development - Add prepare_publish.sh script to convert for publishing - CI workflows now work with workspace mode - Use script before publishing to pub.dev
- Fix empty pubspec.yaml files for macos, windows, linux, web - Restore proper package configuration with workspace mode - All packages now have correct dependencies and plugin config
- Add 'publish_to: none' to all packages - Fixes flutter analyze errors about path dependencies - CI will now pass without warnings - Script prepare_publish.sh will remove this when publishing
- Add 'invalid_dependency: ignore' to analysis_options.yaml - Remove 'publish_to: none' from all packages - Allows path dependencies in development without CI errors - Packages can be published when needed with prepare_publish.sh This enables: - ✅ Development with path dependencies (all branches) - ✅ CI passes without errors - ✅ Publishing to pub.dev when ready (main branch)
- Add 'lines_longer_than_80_chars: false' to all platform packages - Fixes analyzer errors about long import lines - Platform packages have long package names that exceed 80 chars
Summary of ChangesHello @protheeuz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the project's configuration for better monorepo development and prepares its packages for publishing. It adjusts linter rules to be less restrictive, specifically regarding public API documentation and path dependencies, and introduces a dedicated script to streamline the publishing workflow for all sub-packages. A minor documentation update also reflects expanded platform compatibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a script to prepare packages for publishing and updates various configuration files. While the intent to automate the release process is good, the implementation has some issues. My review focuses on a critical bug and other weaknesses in the new prepare_publish.sh script, for which I've provided a refactored version. Additionally, I've commented on the removal of the public_member_api_docs linting rule, which I recommend reinstating to maintain code quality, and the removal of publish_to: none from pubspec.yaml files, which should be handled temporarily by the script instead.
| # Function to update pubspec.yaml for publishing | ||
| update_pubspec() { | ||
| local package=$1 | ||
| local pubspec="packages/$package/pubspec.yaml" | ||
|
|
||
| echo " 📦 Updating $package..." | ||
|
|
||
| # Remove 'resolution: workspace' line | ||
| sed -i.bak '/^resolution: workspace$/d' "$pubspec" | ||
|
|
||
| # Remove 'publish_to: none' line if exists | ||
| sed -i.bak '/^publish_to: none$/d' "$pubspec" | ||
|
|
||
| # Replace path dependency with hosted | ||
| sed -i.bak 's|local_storage_cache_platform_interface:$|local_storage_cache_platform_interface: ^2.0.0|' "$pubspec" | ||
| sed -i.bak '/path: \.\.\/local_storage_cache_platform_interface/d' "$pubspec" | ||
|
|
||
| # Remove backup file | ||
| rm -f "$pubspec.bak" | ||
| } | ||
|
|
||
| # Update all platform packages | ||
| update_pubspec "local_storage_cache_android" | ||
| update_pubspec "local_storage_cache_ios" | ||
| update_pubspec "local_storage_cache_macos" | ||
| update_pubspec "local_storage_cache_windows" | ||
| update_pubspec "local_storage_cache_linux" | ||
| update_pubspec "local_storage_cache_web" | ||
|
|
||
| # Update main package | ||
| echo " 📦 Updating local_storage_cache..." | ||
| sed -i.bak '/^resolution: workspace$/d' "packages/local_storage_cache/pubspec.yaml" | ||
| sed -i.bak '/^publish_to: none$/d' "packages/local_storage_cache/pubspec.yaml" | ||
| sed -i.bak 's|local_storage_cache_platform_interface:$|local_storage_cache_platform_interface: ^2.0.0|' "packages/local_storage_cache/pubspec.yaml" | ||
| sed -i.bak '/path: \.\.\/local_storage_cache_platform_interface/d' "packages/local_storage_cache/pubspec.yaml" | ||
| rm -f "packages/local_storage_cache/pubspec.yaml.bak" | ||
|
|
||
| # Update platform interface | ||
| echo " 📦 Updating local_storage_cache_platform_interface..." | ||
| sed -i.bak '/^resolution: workspace$/d' "packages/local_storage_cache_platform_interface/pubspec.yaml" | ||
| sed -i.bak '/^publish_to: none$/d' "packages/local_storage_cache_platform_interface/pubspec.yaml" | ||
| rm -f "packages/local_storage_cache_platform_interface/pubspec.yaml.bak" |
There was a problem hiding this comment.
This script has a few areas for improvement that make it brittle and hard to maintain:
- Buggy
sedcommand: Thesedcommand on line 23 to replace the path dependency is incorrect. It doesn't account for leading whitespace and would fail to match. The two separatesedcommands could also lead to an invalidpubspec.yamlstate during processing. - Hardcoded version: The version
^2.0.0is hardcoded. This is fragile and will require manual updates whenever thelocal_storage_cache_platform_interfacepackage version changes. - Code duplication: The logic to update
local_storage_cacheandlocal_storage_cache_platform_interface(lines 38-50) is a repetition of the logic inside theupdate_pubspecfunction.
I'd suggest refactoring the script to address these points by making it more robust and maintainable. The suggestion below makes the script DRY, dynamically determines the package version, and correctly performs the pubspec.yaml modifications.
# Get platform interface version dynamically
PLATFORM_VERSION=$(grep '^version:' packages/local_storage_cache_platform_interface/pubspec.yaml | awk '{print $2}')
if [ -z "$PLATFORM_VERSION" ]; then
echo "❌ Could not determine platform interface version." >&2
exit 1
fi
# Function to update pubspec.yaml for publishing
update_pubspec() {
local package=$1
local pubspec="packages/$package/pubspec.yaml"
echo " 📦 Updating $package..."
# Remove 'resolution: workspace' line
sed -i.bak '/^resolution: workspace$/d' "$pubspec"
# Remove 'publish_to: none' line if exists
sed -i.bak '/^publish_to: none$/d' "$pubspec"
# Replace path dependency with hosted, if it exists
if grep -q "path: ../local_storage_cache_platform_interface" "$pubspec"; then
# Use a multi-line sed command to replace the dependency block atomically
sed -i.bak -e "/local_storage_cache_platform_interface:/ { N; s,:\\n\\s*path: \\.\\./local_storage_cache_platform_interface,: ^$PLATFORM_VERSION,; }" "$pubspec"
fi
# Remove backup file
rm -f "$pubspec.bak"
}
# Update all packages by iterating over a list
PACKAGES=(
"local_storage_cache_android"
"local_storage_cache_ios"
"local_storage_cache_macos"
"local_storage_cache_windows"
"local_storage_cache_linux"
"local_storage_cache_web"
"local_storage_cache"
"local_storage_cache_platform_interface"
)
for package in "${PACKAGES[@]}"; do
update_pubspec "$package"
done| name: local_storage_cache | ||
| description: A comprehensive Flutter package for managing local storage and caching with advanced features like encryption, TTL, multi-space architecture, and more. | ||
| version: 2.0.0 | ||
| publish_to: none |
There was a problem hiding this comment.
The publish_to: none line is being removed here, but the new prepare_publish.sh script also handles its removal. To prevent accidental publishing from a development workspace, it's a common and safe practice to keep publish_to: none in the source-controlled pubspec.yaml files. The release script should be responsible for temporarily removing it before publishing. The script's final instruction to run git checkout packages/*/pubspec.yaml also suggests these changes should be temporary. I recommend restoring this line in all modified pubspec.yaml files.
|
|
||
| linter: | ||
| rules: | ||
| public_member_api_docs: true |
There was a problem hiding this comment.
Removing the public_member_api_docs: true rule lowers the bar for API documentation. For a library, especially a platform implementation, having comprehensive documentation for all public members is a best practice that greatly helps consumers of the package. Was this removal intentional? If not, I'd recommend re-instating it. This comment also applies to the other platform packages where this rule was removed.
No description provided.