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

Allow for script to exit with code 0 even if circular dependencies are found #303

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

louisscruz
Copy link

In some cases, one might aim to use madge to get a list of circular dependencies without necessarily exiting 1 in the case that some are found. This PR adds an --allow-circular flag that will allow for that.

@louisscruz
Copy link
Author

@pahen would you consider adding this? Or, perhaps some variation of this?

@PabloLION
Copy link
Collaborator

@@ -265,7 +266,7 @@ function createOutputFromOptions(program, res) {
json: program.json
});

if (circular.length) {
if (circular.length && !program.allowFailures) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (circular.length && !program.allowFailures) {
if (!program.allowFailures && circular.length) {

Kinda niche-picking but in this way it express better which is more important

@@ -37,6 +37,7 @@ program
.option('--stdin', 'read predefined tree from STDIN', false)
.option('--warning', 'show warnings about skipped files', false)
.option('--debug', 'turn on debug output', false)
.option('--allow-circular', 'exit with code 0 when circular dependencies are found', false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.option('--allow-circular', 'exit with code 0 when circular dependencies are found', false)
.option('--allow-circular', 'not exit with code 1 when circular dependencies are found', false)

Describe what's done instead of using its converse. The old description is ambiguous: imagine the case where circular dependencies are found along with other errors. By the old description, madge was meant to exit with 0, which is untrue.

@PabloLION
Copy link
Collaborator

Hi, @louisscruz , thanks for this handy idea. I'm here to help with the PR.
Apart from the aforementioned problems, it seems to me the word "allow" is a bit of vague in the sense of "who allows what": madge won't forbid user to have circular dependency, it only throws error on checking.
Maybe we can consider using word like suppress or ignore?

@PabloLION
Copy link
Collaborator

Due to the inactivity, I'm going to do the edit myself and add some output test.

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

Successfully merging this pull request may close these issues.

2 participants