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 static options_data describing the options for a UI. #13

Closed
wants to merge 1 commit into from

Conversation

clemens-tolboom
Copy link
Collaborator

I need the options avalable through a UI (Drupal).

With the patch done below I get them like done in the (show off image :p)

add-documented-options-2

  • Are the descriptions good enough?
  • Do we need more meta data regarding the options?

Within Drupal I run something like the code below to build the needed UI

  function getMetas() {
    $meta = ClassDiagramBuilder::$options_data;;

    $meta['generate-script'] = array(
      'description' => 'Adds the script for the Class Diagram',
      'default' => FALSE,
    );
    $meta['generate-image'] = array(
      'description' => 'Adds the image for the Class Diagram',
      'default' => TRUE,
    );

    foreach ($meta as $key => &$data) {
      if (is_bool($data['default'])) {
        if ($data['default']) {
          $data['example'] = "$key:0 or 1 (default)";
        }
        else {
          $data['example'] = "$key:1 or 0 (default)";
        }
      }
      else {
        $data['example'] = "$key:" . $data['default'];
      }
    }
    return $meta;
  }

@clue
Copy link
Owner

clue commented Dec 30, 2013

Sorry, but I'd much rather work towards a more SOLID design and aim for single responsibilities. Considering you'd have to add two additional options ("generate-image" and "generate-script") anyway, I think it makes sense to keep all options somewhere around there.

So why not use something along the lines of this?

function getMetas() {
    $meta = array(
        'only-self' => array(
            'default' => true,
            'description' => 'whether to only show methods/properties that are actually defined in this class (and not those merely inherited from base)',
        ),
        'show-private' => array(
            'default' => false,
            'description' => 'whether to also show private methods/properties',
        ),
        'show-protected' => array(
            'default' => true,
            'description' => 'whether to also show protected methods/properties',
        ),
        'show-constants' => array(
            'default' => true,
            'description' => 'whether to show class constants as readonly static variables (or just omit them completely)',
        ),
    );

    $meta['generate-script'] = array(
      'description' => 'Adds the script for the Class Diagram',
      'default' => FALSE,
    );
    $meta['generate-image'] = array(
      'description' => 'Adds the image for the Class Diagram',
      'default' => TRUE,
    );

    foreach ($meta as $key => &$data) {
      if (is_bool($data['default'])) {
        if ($data['default']) {
          $data['example'] = "$key:0 or 1 (default)";
        }
        else {
          $data['example'] = "$key:1 or 0 (default)";
        }
      }
      else {
        $data['example'] = "$key:" . $data['default'];
      }
    }
    return $meta;
  }

@clemens-tolboom
Copy link
Collaborator Author

That's a great suggestion. That gives validation and a general interface too.

But should we use these generate-* too? By a ClassDiagramBuilder->generate(). As that needs to know how to merge ie the image-data and script together.

I use generate-image to create an image tag myself and like to keep it that way as I have full control over that image attributes from within my CMS.

Anyway I'll work your comment.

@ghost ghost assigned clemens-tolboom Dec 30, 2013
@clue
Copy link
Owner

clue commented Dec 30, 2013

I think we might have a misunderstand here :) I was actually trying to suggest that we should not implement this kind of logic in this library.

IMHO the options array was never really a good idea to begin with. Once PR #19 lands and we restructure this library into a ClassVertex and a ClassVertexToGraphVizRecordExporter, we will have to rework those options anyway.

@clemens-tolboom
Copy link
Collaborator Author

/me baffled

Can we get rid of the options then? For as long as we need options I really like this PR (including your suggestion to add ;) these.

Is it OK for now to add these?

@clue
Copy link
Owner

clue commented Dec 30, 2013

Can we get rid of the options then?

I'd very much prefer removing them instead of keep building upon them.

Once PR #19 lands and we restructure this library […]

PR #19 is on my (lengthy) TODO list, so it may take some time until I can refactor this.

@clemens-tolboom
Copy link
Collaborator Author

Maybe we misunderstand each other?

Is setOption here to stay?

    public function setOption($name, $flag)

If no how can it be refactored?
If yes then we should add my initial PR (without generate-*)

Anyway let's then please make a new branch (say 1.x) to save current state as graph-uml is key part of all of my Drupal efforts.

Master is for all our new stuff like refactoring.

@clemens-tolboom
Copy link
Collaborator Author

#20 needs options too and if we put the options were they belong (the class using them) we can easily add more builder without rewriting our script much.

I was 100% positive I've seen this options / meta pattern somewhere but did not manage to find.

For me the bottom line is I don't want, as a CMS coder, to document 'someone else' options.

@clue
Copy link
Owner

clue commented Dec 31, 2013

Is setOption here to stay?

Nope, at least not for too long.

If no how can it be refactored?

Most of this refactoring will take place according to ticket #19. This will result in more classes with a distinguished purpose. So perhaps we will end up with two classes BuilderThatAddsParentsAutomatically and a BuilderThatDoesNotParents.

For me the bottom line is I don't want, as a CMS coder, to document 'someone else' options.

Yeah, I absolutely understand your sentiment, but I don't think there's much we can do about this. After all, you've started adding custom options for image/script export anyway. Besides, we're not really in the position of handling localized strings, which any CMS should be capable of.

You're right that it makes sense to release a tagged version first, I'll address this in ticket #22.

@clemens-tolboom
Copy link
Collaborator Author

After all, you've started adding custom options for image/script export anyway.

That's a misunderstanding ... I did not add generate-* do graph-uml but to Drupal.

Besides, we're not really in the position of handling localized strings, which any CMS should be capable of.

I agree with we are not in the position but not sure why we could not provide the English version.

For now I declare this a no-fix and will see what happens through #20 when issuing a --help.

@clemens-tolboom clemens-tolboom deleted the add-documented-options branch December 31, 2013 14:23
@clemens-tolboom
Copy link
Collaborator Author

I found the code I mentioned used by https://github.com/digitalkaoz/yuml-php/blob/master/src/YumlPhp/Command/ClassesCommand.php#L39

It needs some research but I reopen this PR for now.

@clemens-tolboom clemens-tolboom restored the add-documented-options branch January 10, 2014 16:30
@clemens-tolboom
Copy link
Collaborator Author

Hmmm .. reopen seems no option.

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