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 "Used In" in the html output #60

Closed
wants to merge 11 commits into from
Closed

add an "Used In" in the html output #60

wants to merge 11 commits into from

Conversation

@omansour
Copy link
Contributor

@omansour omansour commented Apr 1, 2013

allowing users to see where the package is used

an optional file can be passed to the command to scan extra repos. written quiet quickly.
thanks

if ($input->getArgument('dependency-file'))
{
// dependecy
$dependency = array();

This comment has been minimized.

@till

till Apr 1, 2013
Contributor

This is only set in the if () but you use it always in L151.

This comment has been minimized.

@omansour

omansour Apr 1, 2013
Author Contributor

indeed txs

squash! avoid notice when no depency file is provided
@@ -41,6 +41,7 @@ protected function configure()
->setDefinition(array(
new InputArgument('file', InputArgument::OPTIONAL, 'Json file to use', './satis.json'),
new InputArgument('output-dir', InputArgument::OPTIONAL, 'Location where to output built files', null),
new InputArgument('dependency-file', InputArgument::OPTIONAL, 'Json fule to use for scanning dependency', null),

This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

fule ?

This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

And it would be better to use an option IMO

}


This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

Please remove these extra lines

@@ -259,6 +286,7 @@ private function dumpWeb(array $packages, OutputInterface $output, PackageInterf
'url' => $rootPackage->getHomepage(),
'description' => $rootPackage->getDescription(),
'packages' => $mappedPackages,
"dependency" => $dependency,

This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

you should use single quotes

This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

btw, as this is an array of dependencies, it would be better to name it dependencies (same for the PHP variable)

@@ -51,16 +51,21 @@ public function doRun(InputInterface $input, OutputInterface $output)
*/
public function getComposer($required = true, $config = null)
{
if (null === $this->composer) {
if (is_null($config)) {

This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

you should use null ===

<th>Used in</th>
<td>
{% for dep_name, dep in dependency %}
<!-- {{ dep_name }} -->

This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

why this HTML comment ?

<td>
{% for dep_name, dep in dependency %}
<!-- {{ dep_name }} -->
{% if (dep_name == name) %}

This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

Looping over the whole array to do something only for one item, for which you know the key ? seriously ?

{% set package_dependencies = attribute(dependency, name) %}
<ul>
    {% for dependency, constraint in package_dependencies %}
        <li>{{ dependency }}</li>
    {% endfor %}
</ul>

This comment has been minimized.

@omansour

omansour Apr 2, 2013
Author Contributor

u right ! this was coded quick and dirty, I'm afraid. I will integrate your notes asap.

thanks

<!-- {{ dep_name }} -->
{% if (dep_name == name) %}
<ul>
{% for dk, d in dep%}

This comment has been minimized.

@stof

stof Apr 1, 2013
Contributor

using meaningful and readable variable names makes it far easier to maintain (see my comment above for instance)

foreach ($packagesDependency as $package) {
$version = $package->getPrettyVersion();
foreach ($package->getRequires() as $link) {
$dependency[$link->getTarget()][$link->getSource()][] = $version;

This comment has been minimized.

@stof

stof Apr 2, 2013
Contributor

This can be simplified a lot:

  • a package cannot have several dependencies with the same target (see how they are defined), so you will never have several packages with the same target and the same source, so using an array of versions is useless (and the version is defined outside the loop anyway)
  • you don't use the pretty version in the template at all
@Seldaek
Copy link
Member

@Seldaek Seldaek commented Apr 2, 2013

As far as I understand, this implements #51 right? Ping @oker1.

@oker1
Copy link
Contributor

@oker1 oker1 commented Apr 2, 2013

@Seldaek, yes it seems so.

@omansour
Copy link
Contributor Author

@omansour omansour commented Apr 2, 2013

please find here an output buid on our private satis

Capture_d cran_02_04_13_21_03

@kennydee
Copy link

@kennydee kennydee commented Apr 3, 2013

👍

1 similar comment
@till
Copy link
Contributor

@till till commented Apr 3, 2013

👍

$packagesDependency = array();

if ($input->getArgument('dependency-file'))
{

This comment has been minimized.

@till

till Apr 3, 2013
Contributor

Crazy nitpick — but this should go on the previous line.

@omansour
Copy link
Contributor Author

@omansour omansour commented Apr 10, 2013

hello, is there anything I can do to this this merged ?

is it usefull or not ?

@bretrzaun
Copy link

@bretrzaun bretrzaun commented Apr 26, 2013

👍

@Seldaek
Copy link
Member

@Seldaek Seldaek commented Apr 27, 2013

@omansour sorry for the delay in giving you feedback. It seems good overall (showing dependencies), but I don't quite understand the point of the additional dependency-file argument? What do you use for that? The way I see it it's either an incomplete feature or an unnecessary one.

@omansour
Copy link
Contributor Author

@omansour omansour commented Apr 28, 2013

@Seldaek no pb :)

the idea behind the dependency-file is to add repos who can appears in the "used in" row but cannot be installed. ie. project that aren't libs or bundle. It's the way i use it at work.

hope it's clear

see u

@Seldaek
Copy link
Member

@Seldaek Seldaek commented Apr 28, 2013

Not so clear no, can you give a concrete example? Also can you tell me what exactly goes in this dependency-file? Is it a composer.json of a project, or a list of packages and their requirements?

@omansour
Copy link
Contributor Author

@omansour omansour commented Apr 28, 2013

is the previous screen I gave, m6/distribution-bundle is referenced in a the standard data/satis.json. So my user can add this bundle in their project as usual. You can see that m6/distribution-bundle is used in the package m6/m6info, which is a project (actually http://www.m6info.fr ). m6/m6info is not referenced in data/satis.json but in data/dependency.json (the famous dependency-file called when I build my satis), so it can appear in the used in row but can't be installed via composer

here the file I use :

{
    "repositories": [
        { "type": "git", "url": "http://git.m6web.fr/minutefacile.git" },
        { "type": "git", "url": "http://git.m6web.fr/polls.git" },
    { "type": "git", "url": "http://git.m6web.fr/log_debug_center.git" },
        { "type": "git", "url": "http://git.m6web.fr/m6info.git" },
    { "type": "git", "url": "http://git.m6web.fr/backstagev3.git" }        ]
}

regards

@omansour
Copy link
Contributor Author

@omansour omansour commented May 2, 2013

hey @Seldaek ! let me know if it's clear or not.

regards
Olivier

@till
Copy link
Contributor

@till till commented May 25, 2013

@omansour Just curious — can you share why this is necessary?

Like, why is the code not installable via composer, or why don't you want to add it to your satis?

@omansour
Copy link
Contributor Author

@omansour omansour commented May 25, 2013

hey

I have created this PR for the same reasons, I guess, #51 has been created. So my take is that can be helpfull for other people.

@Seldaek
Copy link
Member

@Seldaek Seldaek commented May 27, 2013

OK I merged this minus the --dependency-file, because I really don't see the point of having projects that are not installable as such if you are going to track them with composer somehow anyway.

Additionally I added links on the package names so you can quickly go to the package requiring.

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

7 participants