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.

Sign up for GitHub

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

Jump to bottom

resolved Composer #2988

Merged
merged 2 commits into from
Jun 6, 2018
Merged

resolved Composer #2988

merged 2 commits into from
Jun 6, 2018
+46 −1

Conversation

landsman
Copy link
Contributor

fixed pull request in #2803

koenpunt
koenpunt reviewed May 28, 2018
View reviewed changes
.gitignore Outdated
@@ -1,6 +1,7 @@
.DS_Store
node_modules
.project
.idea
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change, should be in your global gitignore file IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just let it here for others contributors?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well since this is specific to a certain IDE I say no, because everyone can decide whatever IDE they use for js development. Was this an Android or iOS project I would agree in adding it to the projects gitignore file, because the options there are limited.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also; https://intellij-support.jetbrains.com/hc/en-us/community/posts/115000154070-How-to-gitignore-idea-files?page=1#community_comment_115000258150

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

composer.json
"homepage": "https://harvesthq.github.io/chosen/",
"require": {
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a trailing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

README.md Outdated
To install with composer:

```
composer require harvesthq/chosen
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to work it should still be added to packagist right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it will after merge.

composer.json
@@ -0,0 +1,36 @@
{
"name": "harvesthq/chosen",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be sure; can the name be an arbitrary value? Because this won't match with the repo path harvesthq/chosen-package.

Copy link
Contributor Author

@landsman landsman May 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that can be it like that.

@landsman
Copy link
Contributor Author

Is it good right now?

@landsman
Copy link
Contributor Author

is it ready to merge please?

tjschuck
tjschuck requested changes May 31, 2018
View reviewed changes
README.md Outdated
@@ -28,6 +28,12 @@ To install with npm:
npm install chosen-js
```

To install with composer:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please capitalize composer and link it to https://getcomposer.org?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@tjschuck
Copy link
Member

I have created a harvesthq account on https://packagist.org in preparation.

@tjschuck tjschuck mentioned this pull request May 31, 2018
Closed
@tjschuck
Copy link
Member

@stof @koenpunt Before we continue too much further, we were all pretty ¯\_(ツ)_/¯ about this on #2803 — do either of you actually have any arguments for or against including this particularly?

I've never heard of Composer/Packagist before, but I haven't written PHP in eons, so that doesn't mean much. It does seem like it's the de facto PHP package manager, though.

That said, Chosen isn't a PHP library. There's also stuff like https://asset-packagist.org that seems to be intended as a bridge between Composer and NPM/bower... is that a better recommendation, or is that not as popular/reliable/trustworthy of a tool?

/cc @harvesthq/chosen-developers

@marclaporte
Copy link

@tjschuck : #2803 (comment) has several examples of JavaScript libs.

We (Tiki Wiki CMS Groupware) use https://asset-packagist.org/ and it's clearly a second choice only when we can't get from Packagist.

I highly recommend to proceed with Packagist:

  • It's a one-time effort, and after, it's all automated
  • PHP developers look on Packagist for solutions. Most won't look on asset-packagist.org
  • asset-packagist.org would be an extra hurdle

Thanks!

@koenpunt
Copy link
Collaborator

koenpunt commented Jun 2, 2018

I used Composer in my PHP days, and there's really no alternative in dependency management. And since it's automated (it uses tagged releases), I don't think this will hurt us.

@landsman
Copy link
Contributor Author

landsman commented Jun 2, 2018 •
edited
Loading

Exactly. It is full-automated solution for PHP world.

We need provide these steps:

  1. merge composer file
  2. create packagist account
  3. connect github repositry with packagist by token
  4. submit repository to packagist
  5. done, automated solution 👍

For example of use-case is port to Drupal Chosen module which need manually install original library to /libraries/ folder. It's annoying if you are use to on automated dependency workflow defined by one json file. After this merge it will be automated for CI/CD.

@tjschuck
Copy link
Member

tjschuck commented Jun 6, 2018

Alright, I'm on board. Thanks, @landsman and @marclaporte !

@tjschuck tjschuck merged commit 57ba728 into harvesthq:master Jun 6, 2018
@tjschuck
Copy link
Member

tjschuck commented Jun 6, 2018

Done! https://packagist.org/packages/harvesthq/chosen

Thanks again, everyone.

@landsman
Copy link
Contributor Author

landsman commented Jun 7, 2018

Thanks @tjschuck !

@JTubex
Copy link

JTubex commented Oct 17, 2019

@hussainweb opened a PR that relates to this one: #3066

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koenpunt koenpunt koenpunt left review comments

@tjschuck tjschuck tjschuck requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@landsman @tjschuck @marclaporte @koenpunt @JTubex