Issue 3745046: WordPress Rewrite Analyzer (1.0)

Can't Edit
Can't Publish+Mail
Start Review

Created:
13 years, 3 months ago by Jan Fabry

Modified:
9 years, 4 months ago

Reviewers:

Base URL:
http://plugins.svn.wordpress.org/

Visibility:
Public.

Patch Set 1 #

Total comments: 24

Created: 13 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+956 lines, -0 lines)			Patch
A	monkeyman-rewrite-analyzer/trunk/include/regex.php	View	1 chunk	+275 lines, -0 lines	6 comments	Download
A	monkeyman-rewrite-analyzer/trunk/include/rewrite-analyzer.php	View	1 chunk	+246 lines, -0 lines	10 comments	Download
A	monkeyman-rewrite-analyzer/trunk/languages/monkeyman-rewrite-analyzer-nl_NL.mo	View	Binary file		0 comments	Download
A	monkeyman-rewrite-analyzer/trunk/languages/monkeyman-rewrite-analyzer-nl_NL.po	View	1 chunk	+177 lines, -0 lines	0 comments	Download
A	monkeyman-rewrite-analyzer/trunk/readme.txt	View	1 chunk	+51 lines, -0 lines	0 comments	Download
A	monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.css	View	1 chunk	+60 lines, -0 lines	2 comments	Download
A	monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.js	View	1 chunk	+65 lines, -0 lines	0 comments	Download
A	monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.php	View	1 chunk	+20 lines, -0 lines	4 comments	Download
A	monkeyman-rewrite-analyzer/trunk/screenshot-1.png	View	Binary file		0 comments	Download
A	monkeyman-rewrite-analyzer/trunk/screenshot-2.png	View	Binary file		0 comments	Download
A	monkeyman-rewrite-analyzer/trunk/ui/rewrite-analyzer.php	View	1 chunk	+62 lines, -0 lines	2 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

Jan Fabry

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.php File monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.php (right): http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.php#newcode15 monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.php:15: //define( 'MONKEYMAN_REWRITE_ANALYZER_FILE', __FILE__ ); Should be MONKEYMAN_REWRITEANALYZER_FILE, or use ...

13 years, 3 months ago (2010-12-29 20:34:47 UTC) #1

Jan Fabry

Comments by Eric http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk/include/regex.php File monkeyman-rewrite-analyzer/trunk/include/regex.php (right): http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk/include/regex.php#newcode1 monkeyman-rewrite-analyzer/trunk/include/regex.php:1: <?php Call this file class.monkeyman_regex.php http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk/include/regex.php#newcode3 ...

13 years, 3 months ago (2011-01-01 14:28:42 UTC) #2

Jan Fabry

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk/include/regex.php File monkeyman-rewrite-analyzer/trunk/include/regex.php (right): http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk/include/regex.php#newcode74 monkeyman-rewrite-analyzer/trunk/include/regex.php:74: if ( ! is_null( $repeat_target ) && preg_match('/\{(\d*)(,?)(\d*)\}(\??)/', $regex, ...

13 years, 3 months ago (2011-01-01 14:34:45 UTC) #3

Jan Fabry

12 years, 11 months ago (2011-05-12 17:25:31 UTC) #4

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
File monkeyman-rewrite-analyzer/trunk/include/regex.php (right):

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/include/regex.php:1: <?php
On 2011/01/01 14:28:42, Jan Fabry wrote:
> Call this file class.monkeyman_regex.php

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/include/regex.php:3: * A utility class that can
parse a regular expression
On 2011/01/01 14:28:42, Jan Fabry wrote:
> More documentation (phpdoc for each class and method), maybe a general
> introduction at the top?

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/include/regex.php:74: if ( ! is_null(
$repeat_target ) && preg_match('/\{(\d*)(,?)(\d*)\}(\??)/', $regex,
$repeat_matches, PREG_OFFSET_CAPTURE, $idx ) && $repeat_matches[0][1] == $idx )
{
On 2011/01/01 14:34:45, Jan Fabry wrote:
> Add space after preg_match(

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
File monkeyman-rewrite-analyzer/trunk/include/rewrite-analyzer.php (right):

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/include/rewrite-analyzer.php:1: <?php
On 2011/01/01 14:28:42, Jan Fabry wrote:
> Call this file class.monkeyman_rewrite_analyzer.php

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/include/rewrite-analyzer.php:1: <?php
On 2011/01/01 14:34:45, Jan Fabry wrote:
> On 2011/01/01 14:28:42, Jan Fabry wrote:
> > Call this file class.monkeyman_rewrite_analyzer.php
> 
> The coding standards suggest class-monkeyman-rewrite-analyzer.php

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/include/rewrite-analyzer.php:2: include_once(
dirname( __FILE__ ) . '/regex.php' );
On 2011/01/01 14:28:42, Jan Fabry wrote:
> Include it here or in the main plugin file?

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/include/rewrite-analyzer.php:199: public
function contextual_help( $contextual_help, $screen_id, $screen )
On 2011/01/01 14:34:45, Jan Fabry wrote:
> Move this to UI too?

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/include/rewrite-analyzer.php:246:
$monkeyman_Rewrite_Analyzer_instance = new Monkeyman_Rewrite_Analyzer();
On 2011/01/01 14:28:42, Jan Fabry wrote:
> Move this to the main plugin file, so this file only contains the class

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
File monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.css (right):

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.css:34: background-color:
#A9E555;
On 2011/01/01 14:28:42, Jan Fabry wrote:
> Use more contrasting colors

Used only matched color, not the green

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
File monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.php (right):

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.php:15: //define(
'MONKEYMAN_REWRITE_ANALYZER_FILE', __FILE__ );
On 2010/12/29 20:34:47, Jan Fabry wrote:
> Should be MONKEYMAN_REWRITEANALYZER_FILE, or use this format everywhere

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/rewrite-analyzer.php:18: if ( is_admin() ) {
On 2011/01/01 14:34:45, Jan Fabry wrote:
> Use a hook here?

Done.

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
File monkeyman-rewrite-analyzer/trunk/ui/rewrite-analyzer.php (right):

http://codereview.appspot.com/3745046/diff/1/monkeyman-rewrite-analyzer/trunk...
monkeyman-rewrite-analyzer/trunk/ui/rewrite-analyzer.php:14: <td><code><?php
echo $url_prefix; ?></code><input id="monkeyman-regex-tester" type="text"
class="regular-text code" /></td>
On 2011/01/01 14:28:42, Jan Fabry wrote:
> Add a button "Reset" or "Clear", to show all rules again

Done.

Expand All Messages | Collapse All Messages