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

Add an option to hide dev depencies in the graph. #19

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

Add an option to hide dev depencies in the graph. #19

wants to merge 1 commit into from

Conversation

@armandabric
Copy link

@armandabric armandabric commented Jun 19, 2014

No description provided.

@clue
Copy link
Owner

@clue clue commented Jun 24, 2014

Thanks @Spy-Seth, the changes look good to me (on a high level) and your PR is very much appreciated 👍

Ideally, I'd like to merge this in, but on a more narrow level I'm a bit concerned with the relation to ticket #2. Before I take a decision, what's your opinion on this matter? So far, it's been my understanding that this should be fixed (upstream) in the underlying library and we should then update our dependencies.

Either way, thanks for your submission!

Loading

@armandabric
Copy link
Author

@armandabric armandabric commented Jun 25, 2014

You're right, schmittjoh/composer-deps-analyzer need to be updated to correctly parse the devDependencies and allow us to safely work with.

Meanwhile, the feature to show or not the dev-dependencies if still a must have feature. So I think this is not bad idea to merge this patch. And when the schmittjoh/composer-deps-analyzer dependency will be fix we could adapt the code with a proper implementation.

Loading

@clue
Copy link
Owner

@clue clue commented Jun 6, 2015

Duh, sorry for the huge delay! I'm okay with getting this in here for now, instead of pushing this upstream. However, I'd prefer to have a reverse option --no-dev in order to be consistent with how composer works nowadays (see also #2).

Can you update this PR?

Loading

@Pierstoval
Copy link

@Pierstoval Pierstoval commented Aug 9, 2015

Hi guys, how are you with this PR? 😃

Loading

@clue
Copy link
Owner

@clue clue commented Aug 9, 2015

@Pierstoval See my last comment :)

I'd be happy to merge this once anybody finishes this 👍

Loading

@armandabric
Copy link
Author

@armandabric armandabric commented Aug 10, 2015

Ho I've forgotten this one. I will try to update the PR this week.

Loading

@armandabric armandabric changed the title Add an option to show/hide dev depencies in the graph. Add an option to hide dev depencies in the graph. Aug 12, 2015
@armandabric
Copy link
Author

@armandabric armandabric commented Aug 12, 2015

I just update the PR!

Loading

@@ -17,13 +17,14 @@ protected function configure()
->setDescription('Show dependency graph image for given project directory')
->addArgument('dir', InputArgument::OPTIONAL, 'Path to project directory to scan', '.')
->addOption('format', null, InputOption::VALUE_REQUIRED, 'Image format (svg, png, jpeg)', 'svg')
/*->addOption('dev', null, InputOption::VALUE_NONE, 'If set, Whether require-dev dependencies should be shown') */;
->addOption('no-dev', null, InputOption::VALUE_NONE, 'Hide dev dependencies.');

Choose a reason for hiding this comment

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

Shouldn't no-dev be replaced by dev as in the Export command?
It would keep consistency between the two commands, therefore ensure better understandability

Loading

Copy link
Owner

@clue clue Aug 12, 2015

Choose a reason for hiding this comment

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

Shouldn't dev be replaced by no-dev? :-) This way it's consistent with how composer works nowadays

Loading

Choose a reason for hiding this comment

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

Indeed it's consistent with composer, but actually, I don't think we need the dev by default. It's an opinion, obviously 😉

Loading

Copy link
Contributor

@keradus keradus Jul 7, 2016

Choose a reason for hiding this comment

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

I would vote for not displaying dev deps by default

Loading

Copy link
Owner

@clue clue Jul 7, 2016

Choose a reason for hiding this comment

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

Care to share some background, what's your motivation for hiding dev dependencies by default?

Here's my list why I think a --no-dev option makes more sense:

  • It's in line with how composer works nowadays
  • Without any options, you see all dependencies; optional arguments can be used to hide, so nothing ever goes unnoticed; this can be confusing, which options need to be passed to show everything?
  • It's in line with how the other future options such as --depth or --purge are going to work
  • Changing the default display options should be considered a BC break

Loading

Choose a reason for hiding this comment

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

i like it being consistent with composer. makes it a no-brainer.

Loading

@clue
Copy link
Owner

@clue clue commented Aug 12, 2015

Awesome!

Please check the above inconsistency, otherwise LGTM 👍

Loading

@armandabric
Copy link
Author

@armandabric armandabric commented Aug 12, 2015

I've updated the PR to fix the inconsistency.

Loading

@Pierstoval
Copy link

@Pierstoval Pierstoval commented Aug 12, 2015

👍 great !

Loading

@ppetermann
Copy link

@ppetermann ppetermann commented Oct 7, 2015

any update on this?

Loading

@clue clue added this to the v1.1.0 milestone Nov 16, 2015
@clue
Copy link
Owner

@clue clue commented Nov 16, 2015

Thanks for the update @Spy-Seth, LGTM 👍 I'll postpone this for now and am looking into getting this into the next release.

Loading

* You may optionally pass an `--format=[svg/svgz/png/jpeg/...]` option to set
the image type (defaults to `svg`).

* By default, all dependecies is show. If you only want to see the production dependencies you can add the `--no-dev` option.
Copy link
Owner

@clue clue Jul 7, 2016

Choose a reason for hiding this comment

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

How about "By default, graph-composer will show all dependencies (require and require-dev keywords). If you want to hide the require-dev dependencies, you can add the --no-dev option." ?

(see also above)

Loading

@clue
Copy link
Owner

@clue clue commented Jul 7, 2016

Thanks for your continuous effort @Spy-Seth, I've just check your changes and would love to get this in 👍

Can you rebase this on the current master? Also, do we have a sane way to add tests for this?

Loading

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

Successfully merging this pull request may close these issues.

None yet

5 participants