Read package.json from APP_ROOT or Rails.root, not cwd #520
Conversation
WalkthroughThe pull request updates the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
lib/shakapacker/utils/manager.rb (1)
38-38: LGTM! Consider a minor optimization.The change to use
Rails.root.join(lock)when checking for lock files ensures that the correct directory is searched, regardless of the current working directory. This addresses the issue mentioned in #519 and aligns with the PR objectives.A minor optimization could be to move the
Rails.root.joincall outside the loop to avoid repeated method calls:MANAGER_LOCKS.find { |_, lock| File.exist?(Rails.root.join(lock)) }&.first || "npm"Could be optimized to:
rails_root = Rails.root MANAGER_LOCKS.find { |_, lock| File.exist?(rails_root.join(lock)) }&.first || "npm"This change is optional and would only provide a very slight performance improvement.
CHANGELOG.md (4)
16-17: Consider adding more context to the Unreleased change.The entry in the Unreleased section is clear, but it could be improved by adding more context about the impact of this change. Consider expanding on how this fix affects users and why it's important.
Here's a suggested expansion:
- Fix error when rails environment is required from outside the rails root directory [PR 520](https://github.com/shakacode/shakapacker/pull/520) + Fix error when Rails environment is required from outside the Rails root directory [PR 520](https://github.com/shakacode/shakapacker/pull/520). This improves the flexibility of Shakapacker, allowing it to function correctly in more diverse project setups and deployment scenarios.🧰 Tools
🪛 Markdownlint
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Line range hint
28-46: Inconsistent bullet point formattingThere's an inconsistency in the formatting of bullet points in this section. Some use "-" while others use "*". For better readability and consistency, it's recommended to use the same bullet point style throughout the document.
Consider updating all bullet points to use the same symbol, preferably "-" to match the majority of the document. For example:
- * This enables support for package managers other than `yarn`, with `npm` being the default; to continue using Yarn, + - This enables support for package managers other than `yarn`, with `npm` being the default; to continue using Yarn,🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Line range hint
48-49: Consider adding a summary for v8.0.0 changesGiven the significance of a major version change and the number of breaking changes, it would be helpful to include a brief summary at the beginning of the v8.0.0 section. This would give users a quick overview of the most important changes before diving into the details.
Consider adding a summary like this:
## [v8.0.0] - May 17, 2024 This major version includes several breaking changes and improvements, focusing on modernizing the package manager support, removing deprecated features, and enhancing overall performance and flexibility. Key changes include switching to `npm` as the default package manager, removing support for older Node.js versions, and various API changes. ### Breaking changes ...🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Line range hint
1-1: Consider creating a historical changelog fileThe CHANGELOG.md file contains entries for many older versions, going back to v5.4.3 and earlier. While this provides valuable historical context, it also makes the file quite long. To improve readability and maintainability, consider moving older version entries (e.g., everything before v6.0.0) to a separate CHANGELOG-HISTORICAL.md file.
You could add a note at the end of the main CHANGELOG.md like this:
... ## v6.0.0 and earlier For changes in version 6.0.0 and earlier, please see [CHANGELOG-HISTORICAL.md](./CHANGELOG-HISTORICAL.md).This approach would keep the main CHANGELOG.md more focused on recent and relevant changes while still preserving the full history of the project.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
spec/shakapacker/utils_manager_spec.rb (2)
94-95: Consolidate stubbing ofRails.rootStubbing
Rails.rootto return the current directory is done in multiple places. To improve consistency and reduce repetition, consider moving this stub to a higher-levelbeforeblock or a shared context that applies to all relevant tests.
170-170: Avoid stubbingRails.rootmultiple timesThe stubbing of
Rails.rooton line 170 is performed in several places throughout the tests. To adhere to the KISS (Keep It Simple, Stupid) principle and improve test clarity, consider moving this stub to a globalbeforeblock or shared context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- lib/shakapacker/utils/manager.rb (2 hunks)
- spec/shakapacker/utils_manager_spec.rb (5 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
🔇 Additional comments (2)
lib/shakapacker/utils/manager.rb (1)
Line range hint
1-58: Summary: Changes effectively address the reported issueThe modifications to
error_unless_package_manager_is_obvious!andguess_binarymethods successfully address the issue reported in #519. By usingRails.rootto locate files, the code now works correctly regardless of the current working directory.These changes align well with the PR objectives and provide a robust solution to the problem. The rest of the file remains unchanged, ensuring that existing functionality is preserved.
Overall, this is a well-focused and effective fix for the reported issue.
CHANGELOG.md (1)
Line range hint
1-1: LGTM with suggested improvementsOverall, the CHANGELOG.md file is well-maintained and provides detailed information about changes across versions. It serves its purpose effectively, helping users understand the evolution of the Shakapacker library and guiding them through upgrades.
The suggested improvements (adding more context to the Unreleased section, consistent bullet point formatting, adding a summary for v8.0.0, and considering a historical changelog file) will further enhance its readability and usefulness.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
| context "when lockfile is in Rails.root, but pwd is different" do | ||
| before do | ||
| rails_root = Pathname.new("rails_root_#{lock}") | ||
| FileUtils.mkdir_p(rails_root) | ||
| FileUtils.touch(rails_root.join(lock)) | ||
| allow(Rails).to receive(:root).and_return(rails_root) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use shared context for consistent test setup
The context "when lockfile is in Rails.root, but pwd is different" in the guess_version method repeats code found in other tests. Implementing a shared context ensures consistent setup and reduces the likelihood of errors due to code duplication.
There was a problem hiding this comment.
If a human reviewer wants me to extract a shared context for something as simple as this, I am more than willing to oblige.
IMVHO, tests profit from a little bit of duplication if it makes the logic easier to follow. Especially rspec shared contexts sometimes make tests unecessarily complicated.
I saw some duplication in the existing code and thought this was the style of the project, but feel free to correct me if I am wrong.
There was a problem hiding this comment.
@marvinthepa, thank you for sharing your perspective. I understand that you prefer some duplication in tests for clarity, and that using shared contexts might make tests unnecessarily complicated. I'll keep that in mind for future reviews.
(\_/)
(•_•) *pondering thoughtfully*
/ \
✏️ Learnings added
Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:145-151
Timestamp: 2024-10-09T10:52:40.755Z
Learning: In tests, prefer some duplication if it makes the logic easier to follow. Avoid using RSpec shared contexts if they make tests unnecessarily complicated.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:68-89
Timestamp: 2024-10-09T10:47:17.620Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, the code within the context `"when lockfile is in Rails.root, but pwd is different"` is not duplicated for each package manager.
|
Once specs pass, I'll merge. |
|
@marvinthepa got a test failure. |
Fixed. In the shakapacker binstub, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
lib/shakapacker/utils/manager.rb (1)
56-66: LGTM! Well-implementedrails_rootmethod.The new
rails_rootmethod provides a robust way to determine the Rails root path, handling different scenarios (presence ofAPP_ROOTorRailsmodule). The method is appropriately marked as private for internal use.Consider slightly modifying the error message for clarity:
- raise "can only be called from a rails environment or with APP_ROOT defined" + raise "Rails.root is not available. Ensure this is called from a Rails environment or APP_ROOT is defined."This change provides more context about what went wrong and what the user can do to resolve the issue.
CHANGELOG.md (2)
Line range hint
7-11: Consider adding a brief description for version 8.0.2The changelog entry for version 8.0.2 lacks a brief description of the changes. Consider adding a short summary to give users a quick overview of what this version includes.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Line range hint
26-69: Consider reorganizing Version 8.0.0 changes for better readabilityThe changelog entry for version 8.0.0 is comprehensive but could be reorganized for better readability. Consider grouping the changes into categories such as "Breaking Changes", "New Features", "Improvements", and "Fixes". This would make it easier for users to quickly understand the impact of this major version update.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- lib/shakapacker/utils/manager.rb (3 hunks)
- spec/shakapacker/utils_manager_spec.rb (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/shakapacker/utils_manager_spec.rb
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
🔇 Additional comments (8)
lib/shakapacker/utils/manager.rb (3)
19-19: LGTM! This change addresses the reported issue.The modification to use
rails_rootwhen reading thepackage.jsonfile ensures that the correct file is accessed regardless of the current working directory. This change directly addresses the issue reported in #519 and aligns with the PR objectives.
38-38: LGTM! Consistent change witherror_unless_package_manager_is_obvious!.The modification to use
rails_root.join(lock)when checking for the existence of lock files ensures consistency with the changes made toerror_unless_package_manager_is_obvious!. This change guarantees that lock files are checked relative to the Rails application's root directory, addressing the issue reported in #519.
Line range hint
19-66: Summary: Effective resolution of the reported issue.The changes in this file successfully address the issue reported in #519 by consistently using
rails_rootto ensure correct file paths are used, regardless of the current working directory. The modifications toerror_unless_package_manager_is_obvious!andguess_binary, along with the addition of therails_rootmethod, provide a robust solution that aligns well with the PR objectives.These changes enhance the reliability of the Shakapacker gem when the Rails environment is required from outside the Rails root directory, which was the core issue reported.
CHANGELOG.md (5)
Line range hint
1-5: LGTM: Header and links sectionThe header section of the changelog is well-structured and provides useful links to upgrade guides for different versions. This is helpful for users navigating between major versions.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Line range hint
19-24: LGTM: Version 8.0.1 changesThe changelog entry for version 8.0.1 is clear and concise, providing good information about the changes made in this version.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Line range hint
71-563: LGTM: Earlier versions and footerThe changelog entries for earlier versions and the footer with version comparison links are well-structured and provide valuable information for users tracking changes across different versions.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Line range hint
1-563: Overall, the CHANGELOG.md is well-maintained and informativeThe changelog provides detailed information about changes across multiple versions of the Shakapacker library. It includes links to upgrade guides, which is very helpful for users. The document is generally well-structured, with a few minor areas for improvement:
- Adding a brief description for version 8.0.2
- Fixing an unordered list style inconsistency in the 8.0.2 section
- Considering a reorganization of the 8.0.0 section for better readability
These minor improvements would enhance the overall quality and usability of the changelog. Great job on maintaining such a detailed and informative changelog!
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
13-17:⚠️ Potential issueFix unordered list style inconsistency
There's an inconsistency in the unordered list style. The Markdownlint tool flagged this issue.
Apply this change to maintain consistency with the rest of the document:
-Corrections have been made to the documentation for the esbuild loader. -- Logic has been added to the SASS rule that is conditional based on the version of the sass-loader. +- Corrections have been made to the documentation for the esbuild loader. +- Logic has been added to the SASS rule that is conditional based on the version of the sass-loader.Likely invalid or redundant comment.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...s since the last non-beta release. ### Changed - Changed internalrequires to `require_relativ...(ENGLISH_WORD_REPEAT_RULE)
🪛 Markdownlint
14-14: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
where did you get APP_ROOT? https://chatgpt.com/share/67085053-11d0-8002-b73a-e337730b805d |
it is defined in the binstubs: shakapacker/lib/install/bin/shakapacker Line 10 in 9544d55 |
justin808
changed the title
|
Thanks @marvinthepa |
Fixes #519.
Summary
We have a few tools that are called from outside the rails root, require the rails environment, and then do something with the rails db.
These fail as error_unless_package_manager_is_obvious only works when called from inside Rails.root.
Other methods like
Shakapacker::VersionChecker::NodePackageVersion.package_json_pathtake this into account, so my guess is that this is just a small bug.Pull Request checklist
Update documentationBugfix, not a documented featureSummary by CodeRabbit
Documentation
New Features
Bug Fixes
Tests