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.

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

read dev-dependencies and get decomposer version #10

Merged
merged 3 commits into from
Mar 18, 2017

Conversation

dducro
Copy link
Contributor

@dducro dducro commented Mar 16, 2017

Get Decomposer version from the package list (default) or from the dependency list if its required by another package.

@dducro
Copy link
Contributor Author

dducro commented Mar 16, 2017

Its also possible to display the dev dependencies.

@introwit
Copy link
Contributor

@dducro thank you for the PR :) Can you please resolve the conflicts as the code structure has changed in the v1.1 release today, so I can merge it. Also:

  • For time being can you remove reading the Dev dependency part because the UI of the view also needs to be planned accordingly.
  • Or if you can do that as well in the PR, it would be great!
  • That improving the getting Decomposer Version part is good to go, just fix the tabs vs spaces thing as per PSR-2 and any other tweaks regarded PSR-2 if any.

Thank you for considering contributing to Decomposer and discussing every possible things.

dducro added 2 commits March 17, 2017 13:09
…into feature/get-decomposer-version

# Conflicts:
#	.gitignore
#	src/controllers/DecomposerController.php
 from package list or from dependency list.
@dducro
Copy link
Contributor Author

dducro commented Mar 17, 2017

I resolved the merge conflicts.

  • I didn't remove reading the dev-dependencies because laravel-decomposer can reside in them (like in our case). If thats the case, it needs to fetch the version from there.
  • I understand that the UI needs updating to reflect these dev-dependencies. Do you have something in mind in mind for the UI? (an extra column or append them at the end of the list with an header separating them or something else? This can be done in a separate PR)
  • I didn't use tabs and I think it conforms to the PSR-2 standard.

@dducro
Copy link
Contributor Author

dducro commented Mar 17, 2017

Much better that you extracted all functionality to it's own class!
One question, why did you make all methods in Decomposer static?
For testing purposes it's much more testable if you use regular methods in stead.
Its pretty easy to fix, do you want me to add this in a separate PR?

@introwit
Copy link
Contributor

@dducro not focusing on testing right now. So other than testing I guess there is no harm in keeping methods static. Will let you know once I feel it's the right time to include tests and I myself will be confident to ask tests from contributors and all of the testing stuff.

@introwit introwit dismissed a stale review March 17, 2017 14:08

Wrong interpretation

@introwit introwit merged commit 4014b39 into lubusIN:master Mar 18, 2017
@dducro dducro deleted the feature/get-decomposer-version branch March 20, 2017 10:36
@dducro
Copy link
Contributor Author

dducro commented Mar 20, 2017

Thanks for merging!

@introwit
Copy link
Contributor

introwit commented Apr 3, 2017

@dducro Hey just fixed the following that I missed earlier, just thought to let you know :)

  • The Problem:
    After this change you made, the getReportArray() broke.

  • The Reason:
    The reason was when we build the associative array for Installed packages we have the key & values as name & version respectively already from the getPackagesAndDependencies() method. But directly passing the Composer Require array which you did breaks because there is no Offset as 'name'.

  • The fix:
    Changed that line back to what it was earlier.

Let me know your thoughts on it :)

@dducro
Copy link
Contributor Author

dducro commented Apr 3, 2017

@introwit Thats true, I probably overlooked that as well.
I changed some stuff to use getDecomposerVersion.
However the call to getPackagesArray is unchanged in this pr.
But it probably should be self::getPackagesArray($packages)
Nice that you found the issue!

(You could consider committing these changes to a develop branch. I couldn't find your fix you just mentioned.)

@introwit
Copy link
Contributor

introwit commented Apr 3, 2017

@dducro I haven't pushed it yet. Just fixed locally and next minute wrote it to you to get your thoughts on it first :)

EDIT: Yeah thinking on that idea to include the flow of pushing to a develop branch in my workflow. Also are you on twitter, so that we can connect quicker in future if needed?

@dducro
Copy link
Contributor Author

dducro commented Apr 3, 2017

Yes, I'm @pindakaas023, not very active on twitter though.

@introwit
Copy link
Contributor

introwit commented Apr 3, 2017

@dducro noted!

@ajitbohra
Copy link
Member

@introwit @dducro we can have gitter chat :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants