-
Notifications
You must be signed in to change notification settings - Fork 140
Remove danger.ts coupling to @markbind/core #2742
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
Remove danger.ts coupling to @markbind/core #2742
Conversation
Refactor to allow clean of same directory and also custom directory if need be
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2742 +/- ##
=======================================
Coverage 52.84% 52.84%
=======================================
Files 130 130
Lines 7162 7162
Branches 1573 1573
=======================================
Hits 3785 3785
Misses 3222 3222
Partials 155 155 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR removes the dependency coupling between the root package and @markbind/core by refactoring the dangerfile to use native console logging instead of MarkBind's logger utility.
- Moves dangerfile.ts from root to scripts directory for better organization
- Replaces @markbind/core logger import with native console calls
- Updates package.json to remove @markbind/core dependency and move danger to devDependencies
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/dangerfile.ts | Replaces logger import with console calls and adds eslint disable comment |
| scripts/clean.js | Extends cleanup functionality to handle compiled files in scripts directory |
| package.json | Removes @markbind/core dependency and moves danger to devDependencies |
| .github/workflows/check-file-diff.yml | Updates dangerfile path and Node.js version |
| .eslintrc.js | Adds eslint override for scripts directory to allow devDependency imports |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| console.warn(`WARN: Detected issues with file couplings:\n${messages.join('\n')}`); | ||
| console.warn('WARN: Please ensure implementation changes are accompanied ' | ||
| + 'by corresponding test or documentation updates.'); | ||
| } else { | ||
| logger.info('All file couplings are correctly updated.'); | ||
| console.info('info: All file couplings are correctly updated.'); |
Copilot
AI
Aug 25, 2025
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.
[nitpick] The log messages now include hardcoded prefixes like 'WARN:' and 'info:' which creates inconsistent formatting. Consider using a simple helper function for consistent log formatting or stick to the native console methods without prefixes.
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.
I added the prefixes as the CLI on github actions doesn't show a differentiation in color or otherwise for warn/info/error.
#2523
Ultimately, the end state would be to directly use .warn(), .info() from the danger import itself. So there wouldn't be native console methods. So I am fine with doing it this way for now.
Once the Danger integration stabilizes, these should be replaced with direct calls to danger.warn() and danger.info()
for consistent reporting within the Danger system.
AgentHagu
left a comment
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.
LGTM! As you've already mentioned, once Danger integration looks finished, we can use danger's API for warnings and info messages. We could also look into whether we want to use Danger.fail instead of .warn for the coupling issues.
For now, we should create a follow-up issue on tracking the accuracy of the coupling detection as mentioned in #2709.
|
Have made a follow up issue #2744 for continuity of this feature! wil merge this refactor PR and close the issue. |
What is the purpose of this pull request?
Overview of changes:
Fixes #2708
@markbind/coredependency from rootpackage.jsondangerfile.tsto remove import of logger, use simpleconsolecalls indangerfile.ts.dangerto devDependency since it's not a dependency.I also updated the clean.js script to clean any
.jsfiles in the/scriptsdirectory if thedangerfile.tsis compiled intojs. Though it does not have to be compiled tojsas it works directly passing thedangerfile.tstodanger.Anything you'd like to highlight/discuss:
No functional difference between using MarkBind's logger wrapper (colored):
console.info: https://github.com/MarkBind/markbind/actions/runs/16864511173/job/47769516895?pr=2742#step:5:7Testing instructions:
Check no regressions.
Proposed commit message: (wrap lines at 72 characters)
Remove @markbind/core dependency from root package.json
Refactor dangerfile.ts to replace logger import from @markbind/core
with native console calls, reducing unnecessary dependency overhead.
Additionally, move dangerfile.ts to /scripts directory from root.
Modularize clean.js function to clean compiled js files from
dangerfile.ts in the /scripts folder as well. Also update CI workflow.
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):