Skip to content

Refactoring and coding improvement #5

Closed
wants to merge 13 commits into from

4 participants

@mauris
mauris commented Dec 17, 2012
  • better compliance with PSR-0 and PSR-1 accepted guidelines.
  • installation of PDC
  • better testing through PHPUnit fixtures and abstract parser testing
  • test fixes in commit 90c3e2b
@simensen
dflydev member

@thephpdeveloper I'll need to think on this.

dflydev/markdown is currently in use by a lot of people and this will be a huge BC break. I think these changes would be good for a 2.x line, but I am hoping to not maintain dflydev/markdown long enough to make it to 2.x. The correct solution would be to get @michelf to finish the lib branch of the actual PHP-Markdown source so we do not have to continue to maintain a fork.

See the WIP from @michelf here:

https://github.com/michelf/php-markdown/tree/lib

@mauris
mauris commented Dec 17, 2012

I see.

The changes I've made weren't so great to be a 2.x. At least it'd be 1.1.x. I understand about the BC break.

The main code base for Markdown parsing in MarkdownParser and MarkdownExtraParser were left intact. The major changes for this PR is to allow developers to write less code with the renaming of:

  • dflydev\markdown\MarkdownParser to DflyDev\Markdown\Parser
  • dflydev\markdown\MarkdownExtraParser to DflyDev\Markdown\ExtraParser
  • dflydev\markdown\IMarkdownParser to DflyDev\Markdown\IParser
  • $markdown->transformMarkdown() to $markdown->transform()

This is a good repository and I definitely hope even if you don't wish to continue maintaining, you'd accept PRs from other developers. Otherwise I wish to obtain your permission to maintain this project by forking into packfire/markdown and I'll see what I can work on from here onwards.

@mauris
mauris commented Dec 17, 2012

@simensen great thought by the way. I didn't realise you were manually maintaining as a detached fork.

What I can do is to fork @michelf's markdown repository and work ONLY on the lib branch.

I will merge in changes from your repository, of course with your permission, so that we have

  1. Unit testing with Travis CI integration
  2. A fully working Composer meta-data (i've seen the attempt)
  3. A better OO approach with abstraction (like how you implemented it)
  4. the best of two worlds.
@simensen
dflydev member

@thephpdeveloper I would suggest you talk to @michelf before doing a lot of work in any branch there just to make sure that the work will be welcomed and that you do it in the correct branch. Otherwise, by all means, go for it... take whatever you like back in that direction.

@mauris
mauris commented Dec 17, 2012

@simensen thank you so much. Great work you've done here.

I'll talk to @michelf to see what we can work out.

@simensen
dflydev member

@thephpdeveloper thanks. :)

@luxifer
luxifer commented Dec 18, 2012

great work

@mauris mauris referenced this pull request in michelf/php-markdown Dec 18, 2012
Closed

Composer Package #31

@lyrixx lyrixx commented on the diff May 1, 2013
.gitignore
@@ -1,2 +1,2 @@
vendor
-composer.lock
+composer.phar
@lyrixx
lyrixx added a note May 1, 2013
  1. As this is a 3rd party lib, you should ignore the composer.lock.
    You should revert your change.

  2. as composer.phar is not part of this project, you may not exclude it. If I can give you an advice, you should install composer.phar globally (see https://getcomposer.org/doc/00-intro.md#globally) or ignore all composer.phar like that: https://github.com/lyrixx/dotfiles/blob/master/.gitconfig#L42-L44

@mauris
mauris added a note May 1, 2013

@lyrixx

  1. 3rd party lib or not, composer.lock MUST NOT be ignored. During Composer install, the dependencies of this library will be installed according to what has been prescribed in the lock file. Without the lock file, the developers of this lib has no control over what version of dependencies the end-users can ultimately install. Read up on thephpdeveloper/dflydev-markdown@f4f7170

  2. my composer is installed globally. I like to ignore them locally too. i can ignore all composer.phar by global ignorelist too. no problem with that.

@lyrixx
lyrixx added a note May 2, 2013

3rd party lib or not, composer.lock MUST NOT be ignored. During Composer install, the dependencies of this library will be installed according to what has been prescribed in the lock file

This is wrong, See : http://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file (the blue block) or http://getcomposer.org/doc/02-libraries.md#lock-file

Note: This is fun, this is the same link. But I think you did not read everything.

Without the lock file, the developers of this lib has no control over what version of dependencies the end-users can ultimately install.

Yes, it is possible thanks to composer.lock. Everything is in the doc, but if you want, I can explain that here.

@mauris
mauris added a note May 2, 2013

It's even more interesting that you skipped the second sentence.

From http://getcomposer.org/doc/02-libraries.md#lock-file:

This can help your team to always test against the same dependency versions.

It's a trouble if you're unable to control your test environment.

@lyrixx
lyrixx added a note May 2, 2013

It's a trouble if you're unable to control your test environment.

Why not. But since the end developer have no control on dependencies of your lib ... This is irrelevant.

@simensen
dflydev member
simensen added a note May 2, 2013

I'm sorta on the fence on this one. I generally add composer.lock to .gitignore for my library projects but I've seen some decent arguments for adding composer.lock to the repository.

The main reason I've seen for including composer.lock for library projects is that it can help in testing by ensuring that anyone else doing development work on the library can have the same dependencies. While this is extremely important for applications I think that this is far less critical for library projects since by nature of Composer the dependencies the library is installed against is a moving target anyway. Pinning a library to a specific set of dependencies doesn't really do a whole lot to help aid development when you look at it from that perspective.

In any event, the point is moot as I don't really see this PR getting merged. :) Feel free to discuss these things further if you want to but please keep that in mind.

If you disagree and think that I should consider more changes to this fork and/or if you think I should reconsider merging this PR, let's discuss that some more on the PR itself and not on this particular code comment thread.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lyrixx lyrixx commented on the diff May 1, 2013
README.md
Simple usage for the Markdown Extra ([details](http://michelf.com/projects/php-markdown/extra/)) parser:
<?php
- use dflydev\markdown\MarkdownExtraParser;
+ use DflyDev\Markdown\ExtraParser;
@lyrixx
lyrixx added a note May 1, 2013

IMHO, use use DflyDev\Markdown\ExtraParser as Parser is easier and more flexible.

@mauris
mauris added a note May 1, 2013

possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lyrixx lyrixx commented on the diff May 1, 2013
phpunit.xml.dist
@@ -12,16 +12,13 @@
>
<testsuites>
<testsuite name="dflydev-markdown Test Suite">
- <directory>./tests/dflydev/</directory>
+ <directory suffix="Test.php">tests</directory>
@lyrixx
lyrixx added a note May 1, 2013

suffix="Test.php" is useless here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lyrixx lyrixx commented on the diff May 1, 2013
src/DflyDev/Markdown/IParser.php
-interface IMarkdownParser {
+/**
+ * Provides an interface of what a Markdown parser in PHP can do
+ */
+interface IParser {
@lyrixx
lyrixx added a note May 1, 2013

As you break all BC, can you rename ParserInterface ? it more PSR* compliant.

@mauris
mauris added a note May 1, 2013

whether IParser or ParserInterface, this is up to the discretion of the developers. PSR doesn't govern over this naming issue.

If it does, kindly point it out to me from the PSR (:

@lyrixx
lyrixx added a note May 2, 2013

You are right, PSR does not enforce you.
But in the sample https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md#11-example they use FooBarInterface. It is the same in symfony, silex, ...

@mauris
mauris added a note May 2, 2013

Again like I said, it's up to the discretion of the developers. Maybe the developers feel that IFooBar is shorter than FooBarInterface and hence makes code reading less cluttered.. Then again, IIterator is not any more illegible than IteratorInterface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lyrixx lyrixx commented on the diff May 1, 2013
tests/DflyDev/Markdown/BaseParserTest.php
- /**
- * Create a markdown parser.
- * @param array $configuration Optional configuration
- * @return \dflydev\markdown\IMarkdownParser
- */
- public function createParser($configuration = null)
- {
- if ( $configuration !== null ) {
- return new \dflydev\markdown\MarkdownParser($configuration);
- }
- return new \dflydev\markdown\MarkdownParser();
- }
+ protected $object;
@lyrixx
lyrixx added a note May 1, 2013

Can you rename object in parser ? explicit is better than implicit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lyrixx lyrixx commented on the diff May 1, 2013
tests/bootstrap.php
@@ -8,5 +8,5 @@
* file that was distributed with this source code.
*/
-$loader = require dirname(__DIR__).'/vendor/autoload.php';
-$loader->add('dflydev\\tests\\markdown', 'tests');
+$loader = require(dirname(__DIR__) .'/vendor/autoload.php');
+$loader->add('DflyDev\\Markdown', array('src', 'tests'));
@lyrixx
lyrixx added a note May 1, 2013

this file can be removed since everything can be managed in composer.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lyrixx
lyrixx commented May 1, 2013

There is a lot a BC break. But we can add a thin layer to not break BC. I meant we can add some class alias and method alias. with that, this PR can stay BC. and, for the 1.1, or 2.0, we will remove theses alias.
We did the same thing in symfony

@simensen
dflydev member
simensen commented May 1, 2013

@lyrixx Thanks for taking a look at this PR. I had forgotten it was open. Since michelf has now fully embraced Composer, I do not plan to continue maintaining this fork. I think this is best for everyone. :)

https://packagist.org/packages/michelf/php-markdown

There is a lot a BC break. But we can add a thin layer to not break BC. I meant we can add some class alias and method alias. with that, this PR can stay BC. and, for the 1.1, or 2.0, we will remove theses alias.

I'd be willing to discuss it but I'd just as soon have everyone move to using michelf/php-markdown since it is available now. Do you think this is a reasonable goal?

@lyrixx
lyrixx commented May 1, 2013

yes ;)

@mauris
mauris commented May 1, 2013

@lyrixx @simensen I too had forgotten about this. Thanks for bringing this up.

I agree with @simensen, we should re-consolidate everything again with @michelf.

However I must remind, BC break is not a bad thing. Sometimes, it is a good, and hopefully the right, thing to do because it helps to improve code a lot more. Let's not see BC break as though it is some kind of plaque, but embrace it with constructive criticism.

@simensen
dflydev member
simensen commented Jan 6, 2014

I am closing this. This package is now deprecated. Thanks for taking the time to discuss this here. I appreciate everyone's support!

@simensen simensen closed this Jan 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.