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

Configuration not work #8

Closed
mostofreddy opened this issue Jan 30, 2014 · 7 comments
Closed

Configuration not work #8

mostofreddy opened this issue Jan 30, 2014 · 7 comments
Milestone

Comments

@mostofreddy
Copy link
Contributor

Using the example in the documentation, directory, phar and target configuration not working.

The correct configuration is:
phpdocumentor: {
dist: {
options: {
directory : './libs',
phar: null
}
}
}
or modify code to detect both configurations

@bgaillard
Copy link
Member

Hi @mostofreddy and thank you for this contribution,

As its explained in the Grunt documentation Grunt options I think the plugin should support the following :

  • Task level options
  • Specific target options (overwrites task level options)
  • No options defined (uses the defaults)

Your pull request seems good and I'll integrate it and test it in version 0.4.1.

For future releases of the plugin I'm wondering if specifying the options without the options property is the correct way to configure a Grunt plugin :

phpdocumentor: {
    dist: {
        directory : './libs',
        phar: null
    }
}

Is it compliant with Grunt recommendations / Grunt common practices ?

Perhaps your proposition (use the options property) should be the only way to provide configuration with the plugin ?

phpdocumentor: {
    dist: {
        options : {
            directory : './libs',
            phar: null
        }
}

What do you think about that ?

Thanks,

Baptiste

@Melindrea
Copy link

I agree with the options property, though as long as the configuration works I'm fine with either =)

Do you have plans on integrating the ignore option?

@bweston92
Copy link

I am also having issues!

The issue lies here: (https://github.com/gomoob/grunt-phpdocumentor/blob/master/tasks/lib/phpdocumentor.js#L80)

options = runner.options(defaults);

The runner contains the options but after this method is called the options have dropped.

@bweston92
Copy link

Sorry I see that you need to specify 'dist' -> 'options' yet the readme says 'dist'.

@bgaillard
Copy link
Member

After reflection I think this declaration should be considered invalid :

phpdocumentor: {
    dist: {
        directory : './libs',
        phar: null
    }
}

Supporting the 2 formats (with and without the options object) is risky and will complexify the code when we'll need to support more phpDocumentor functionalities later.

So I propose to only update the documentation with :

phpdocumentor: {
    dist: {
        options: {
            directory : './libs',
            phar: null
        }
    }
}

We could also add a more complex sample with multiple Grunt targets and a common Task level configuration in the documentation for the 0.4.1 release.

grunt.initConfig({
    phpdocumentor: {

        // Place here Task level options (i.e common to all your phpDocumentor targets)
        options : {
            command : 'run',
        },

        // Grunt Target used to generate a first documentation
        first_api_documentation : {
            options: {
                directory : 'src/first_api',
                target : 'docs/first_api_documentation'
            }
        },

        // Grunt target used to generate a second documentation
        second_api_documentation : {
            options : {
                directory : 'src/second_api',
                target : 'docs/second_api_documentation'
            }
        }, 

        // Sample target used to display the phpDocumentor help
        display_help : {
            options : {
                command : 'help'
            }
        }

    }
})

Thanks

bgaillard added a commit that referenced this issue Feb 1, 2014
Add a unit test to test Task options overwriting
@bgaillard
Copy link
Member

Documentation fixed in version 0.4.1, thanks for your involvement !

@bgaillard
Copy link
Member

@Melindrea

Do you have plans on integrating the ignore option?

Yes, @mostofreddy provided a pull request #10 to support all available options for the run command few days ago.

I'm planning to test it and integrate it for the 0.5.0 release.

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

No branches or pull requests

4 participants