search

wpsharks / wp-kb-articles

KB Articles for WordPress; adds a new Custom Post Type.
http://wpkbarticles.com/
2 stars 1 forks source link

Class for GitHub API #11

Closed brucewrks closed 9 years ago

brucewrks commented 9 years ago

Referencing #2

Checklist

The class leverages the GitHub REST API via WP_Http. The class only uses 3 endpoints and therefore the use of one of the (unofficial) GitHub SDK libraries has been skipped.

This class is reusable and can be used for multiple queries to the same, or different repositories using the same authentication method.

Usage

$git_api = new \wp_kb_articles\github_api;
$git_api->set_owner('repo_owner');
$git_api->set_repo('repo');
// $git_api->set_branch('000000-dev'); // (optional, defaults to the repo's default branch)
// $git_api->authenticate('user', 'password_or_api_key'); // (optional, will allow for up to 5000 requests, hourly, instead of 60)

// $posts = $git_api->retrieve_posts(); // To retrieve all Markdown file info without Post body or content
$posts = $git_api->retrieve_posts(true); // To retrieve all Markdown files with YAML headers and content

// $post = $git_api->retrieve_post('/path/to/file'); // To retrieve a single Post
jaswrks commented 9 years ago

@brucewsinc You're still working on this right? Just checking :-) I see there are a few checkboxes that you have left for this, which is fine. I just want to be sure that you're not waiting on me to merge this.

raamdev commented 9 years ago

I just want to be sure that you're not waiting on me to merge this.

@jaswsinc @brucewsinc For tracking the state of Pull Requests, I suggest creating a green Issue label called PR: ready to merge and a yellow label called PR: in progress that we can use to identify when a PR is ready for merging and when it's being worked on. This would be especially helpful when someone other than the lead developer (i.e., the person in charge of merging PRs) is working on a PR and the lead dev needs to know when a PR is ready.

brucewrks commented 9 years ago

@jaswsinc

You're still working on this right?

Right.

brucewrks commented 9 years ago

At this point, the class should be fully functional in most cases. The last 2 checkboxes shouldn't be necessary under normal circumstances, and can be added later.

jaswrks commented 9 years ago

At this point, the class should be fully functional in most cases. The last 2 checkboxes shouldn't be necessary under normal circumstances, and can be added later.

@brucewsinc Is this ready to merge then? You said, "fully functional"; but then said, "most cases". Does that mean it is ready or not?

@brucewsinc In the original specs I remember stating the following...

Any values that should be pulled from a config file should exist as configurable class properties; i.e. making it possible to instantiate the class with a certain set of credentials, or with a certain repo URL, etc. Any of these should be a part of the class constructor.

Things like set_owner and set_repo. Please work to do away with those and instead just allow the class to be instantiated with an array of possible options. This will reduce the size of the class file and we can just work with a class instance instead of needing to learn each of the setters/getters.

<?php
public function __construct(array $args = array())
{
  $default_args = array(
    'owner' => '',
    'repo' => '',
   ...
  );
  $args = array_merge($default_args, $args);
  $args = array_intersect_key($args, $default_args);
  $this->repo = $args['repo']; // etc...
}
jaswrks commented 9 years ago

@brucewsinc Please add docBlocks for each class member, including any class properties.

jaswrks commented 9 years ago

@brucewsinc Please establish which of the methods in this class are public (i.e. for API consumption) and which of them are there for internal use only. Internal class members (whether they be methods or properties) should be protected or private please.

What you should end up with is a class which exposes one or two public function declarations where it's easy to see that these are methods that you intend to offer as part of the API made possible through the implementation of this class. Anything else can be protected.

protected vs. private

I generally do not use private at all. Private methods cannot be easily extended by another class. The keyword protected is adequate in most cases and it's what I prefer to use in this plugin.

jaswrks commented 9 years ago

@brucewsinc I see there is a property in this class with the name apiKey that uses camelCase, while all of the other methods and property names use lowercase with underscores. I'd like to stick with lowercase with underscores in this plugin, so please change apiKey to api_key.

jaswrks commented 9 years ago

@brucewsinc On this line, if the body starts with ---, strpos will return 0 and this will not work properly? Is that correct? If so, then I'd suggest that you test the return value of strpos() with !== FALSE to avoid that scenario, which seems rather common to me; i.e. that the body will start with ---.

https://github.com/websharks/wp-kb-articles/blob/0390709b3806f1dbb94ef221e54184b8ae04cd93/wp-kb-articles/includes/classes/github-api.php#L166

jaswrks commented 9 years ago

@brucewsinc Regarding the line that looks for the end...

$endsAt   = strpos($body, '---', $startsAt);

What if there is a title or slug that contains --- also? I think a better approach here would be to look for a line that starts with ---, and not necessarily anything in between the markers.

<?php

$string = '
---
name1: ---value1---
name2: ---value2---
name3: ---value3---
---
';

$headers                   = array();
$_inside_yaml_front_matter = FALSE;
foreach(explode("\n", str_replace(array("\r\n", "\r"), "\n", $string)) as $_line)
{
    if(!($_line = trim($_line)))
        continue; // Empty line.

    if(!$_inside_yaml_front_matter && strpos($_line, '---') === 0)
    {
        $_inside_yaml_front_matter = TRUE;
        continue; // Continue to next line.
    }
    else if($_inside_yaml_front_matter && strpos($_line, '---') === 0)
    {
        $_inside_yaml_front_matter = FALSE;
        break; // All done with YAML front matter.
    }
    if($_inside_yaml_front_matter && strpos($_line, ':', 1) !== FALSE)
    {
        list($_name, $_value) = explode(':', $_line, 2);
        $headers[strtolower(trim($_name))] = trim($_value);
    }
}
unset($_inside_yaml_front_matter, $_line, $_name, $_value);

print_r($headers);
brucewrks commented 9 years ago

Is this ready to merge then? You said, "fully functional"; but then said, "most cases". Does that mean it is ready or not?

If the repo has too many files, the initial ?recursive=1 API call to GitHub could fail to retrieve all of the files in the repo. It's unclear what the limit is, but that needs to be finished before the plugin is complete.

Things like set_owner and set_repo. Please work to do away with those and instead just allow the class to be instantiated with an array of possible options.

Gotcha. Working on this now.

I see there is a property in this class with the name apiKey that uses camelCase...

I'll work on this as well and push it in my next commit.

jaswrks commented 9 years ago

@brucewsinc Here is an improved example with comments. Here is a runnable where you can see it in action. http://runnable.com/VKzXbql-B9k-I86w/yaml-front-matter-simple-parser-w-o-lib-dependencies-for-php

<?php
/*
Example input string.
*/
$article = '
---
name1: ---value1---
name2: ---value2---
name3: ---value3---
---

Body.. this is an article that I wrote.
It is quite comprehensive eh? lol
';

print_r(parse_article($article));

/*
Array
(
    [headers] => Array
        (
            [name1] => ---value1---
            [name2] => ---value2---
            [name3] => ---value3---
        )

    [body] => Body.. this is an article that I wrote.
It is quite comprehensive eh? lol
)
 */

/* ---------------------------------------------------------------------------- */

/**
 * Parses a KB article w/ possible YAML front matter.
 *
 * @param string $article Input article content to parse.
 *
 * @return array An array with two elements.
 *    - `headers` An associative array of all YAML headers.
 *    - `body` The body part of the article after YAML headers were parsed.
 */
function parse_article($article)
{
    // Initialize parts to return here.
    $parts = array(
        'headers' => array(),
        'body'    => '',
    );
    // Normalize line breaks. Use "\n" for all line breaks.
    // e.g. convert old Mac-style line breaks, or Windows-style line breaks.
    // See also: <http://en.wikipedia.org/wiki/Newline>
    $article = str_replace(array("\r\n", "\r"), "\n", $article);
    $article = trim($article); // Trim any leading/trailing whitespace.

    // If the article does NOT start with `---` (on its own line), it contains no YAML front matter.
    // In this case, we should return the full article as the body. There are no headers.
    if(strpos($article, '---'."\n") !== 0)
    {
        $parts['body'] = $article;
        return $parts;
    }
    // Split on lines that contain only `---`.
    // We expect two lines that contain only `---` for a total of three string parts.

    // Note that Markdown allows for `---` also, so the check above is important;
    //    i.e. the article must start with `---` on a separate line for us to get here.
    $article_parts = preg_split('/^\-{3}$/m', $article, 3);

    // If the article does NOT have three parts, it contains no YAML front matter.
    // In this case, we should return the full article as the body. There are no headers.
    if(count($article_parts) !== 3)
    {
        $parts['body'] = $article;
        return $parts;
    }
    // Assign each of the parts; giving them proper names here to make it easier to work with.
    // Note that the first part in this array is the part before the first `---`; which is useless to us.
    list(, $article_headers_part, $article_body_part) = $article_parts;

    foreach(explode("\n", trim($article_headers_part)) as $_line) // Iterate lines in the headers part.
    {
        if(!($_line = trim($_line))) // Trim this line.
            continue; // Skip over empty lines; i.e. with whitespace only.

        if(strpos($_line, ':', 1) !== FALSE)
        { // If this line contains `:` with at least one char in front of `:`.
            list($_name, $_value) = explode(':', $_line, 2); // Break `name: value` into two parts.
            $parts['headers'][strtolower(trim($_name))] = trim($_value); // Add it to the array of headers.
        }
    }
    unset($_line, $_name, $_value); // Housekeeping.

    $parts['body'] = trim($article_body_part); // Trim the body part parsed above already, and include it in the return array.

    return $parts;
}
brucewrks commented 9 years ago

@jaswsinc

Here is an improved example with comments.

Thanks! That makes a lot more sense. I'm implementing this function now.

jaswrks commented 9 years ago

Great! Don't forget to alert me whenever this is ready to merge. You can add a "ready to merge" label and just post an update that explicitly says, "ready to merge". I'll come back and do a final review then and get it merged in.

brucewrks commented 9 years ago

@jaswsinc This is ready to be reviewed and merged.

jaswrks commented 9 years ago

Merged! Great work @brucewsinc :+1: