lumenlearning / candela-citation

Citations for Candela
MIT License
5 stars 4 forks source link

adding composer, apply coding standards #2

Closed bdolor closed 7 years ago

bdolor commented 7 years ago

I would like to include candela-citation via composer but require a composer.json file at the root of this plugin.

"repositories": [
    {
      "type": "vcs",
      "url": "https://github.com/lumenlearning/candela-citation.git"
    }
  ],
  "require": {
    "lumenlearning/candela-citation": "^0.2.1"
  },

No functionality will change as a result, I have merely moved things around to adhere to coding standards.

bracken commented 7 years ago

seems fine to me, I'll let @monkecheese look this over though when he's back from vacation soon.

bdolor commented 7 years ago

If you would consider generating a release as well, please. https://help.github.com/articles/creating-releases/

ghost commented 7 years ago

@bdolor, i'm getting an error when I run composer install:

Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. Run update to update them.
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for doctrine/instantiator 1.1.0 -> satisfiable by doctrine/instantiator[1.1.0].
    - doctrine/instantiator 1.1.0 requires php ^7.1 -> your PHP version (7.0.19) does not satisfy that requirement.
  Problem 2
    - doctrine/instantiator 1.1.0 requires php ^7.1 -> your PHP version (7.0.19) does not satisfy that requirement.
    - phpunit/phpunit-mock-objects 3.4.4 requires doctrine/instantiator ^1.0.2 -> satisfiable by doctrine/instantiator[1.1.0].
    - Installation request for phpunit/phpunit-mock-objects 3.4.4 -> satisfiable by phpunit/phpunit-mock-objects[3.4.4].

The problem seems to be the composer.lock file. When I run composer update, it all works as expected.

bdolor commented 7 years ago

I gather the difference between the php version (7.1) in my development env and yours (7.0) is the source of the problem. PHPCS is only a development requirement and you could take it out if you wanted. I've made assumptions about what tools you use in your workflow, so up to you if you want to keep them or not. I could resubmit by switching my php version to 7.0, keep the dev requirement in there, composer update then re-commit, or I could get rid of the dev requirement altogether and re-submit, or you could accept PR and perform one of those steps yourself.

I discovered over the weekend that this PR will also require you to change references from CandelaCitation::renderCitation as found in https://github.com/lumenlearning/candela/blob/fe058292955bce9d568441182a274237aae6e910/wp-content/plugins/candela-utility/themes/bombadil/single_page.php#L19 to \Candela\Citation::renderCitation which may be a deal breaker for you.

ghost commented 7 years ago

Okay that makes sense. Yes, I'd be happy to accept the PR if you could switch to php 7.0, composer update, and re-commit.

Thanks for mentioning the reference change needed as well. I saw that when I was testing and have the appropriate change on deck for that. Cheers @bdolor!

bdolor commented 7 years ago

let me know if there's anything else