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

Support drupalorg packages #31

Merged
merged 29 commits into from
Apr 26, 2024

Conversation

darthsteven
Copy link
Contributor

This is an alternative to #28 that should also fix #24, well, let's call it a work-in-progress.

In my very limited testing this seems to work with my composer diffs.
I've not actually worked out how to make the actual tests pass mind you.

@darthsteven
Copy link
Contributor Author

I've added some tests, but I'm really sorry, my dev environment isn't set up for writing composer extensions, much less testing them! So it looks like the tests pass, but that's because they are pretty trivial now.

@darthsteven darthsteven marked this pull request as ready for review April 23, 2024 05:58
Copy link

codecov bot commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 76.47059% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 98.73%. Comparing base (57c9f41) to head (cb1c968).

❗ Current head cb1c968 differs from pull request most recent head e2a7c06. Consider uploading reports for the commit e2a7c06 to get more accurate results

Files Patch % Lines
src/Url/DrupalGenerator.php 75.75% 8 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##                main      #31      +/-   ##
=============================================
- Coverage     100.00%   98.73%   -1.27%     
- Complexity       206      221      +15     
=============================================
  Files             19       20       +1     
  Lines            610      634      +24     
=============================================
+ Hits             610      626      +16     
- Misses             0        8       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@IonBazan
Copy link
Owner

Thanks @darthsteven for picking this up! Sorry this PR got lost in my notification inbox. The code looks good at the first glance and it's great that you added tests too. I'll try to review and test this locally in the next couple of days.

Please don't worry about failing CI, the coding standards are set extremely high for this repository so I will fix them for you later.

@IonBazan
Copy link
Owner

BTW if you wish to test it with a real-life project, the easiest way to do so is to navigate to that project directory and run the composer-diff binary from there.

For example, if your project is in ~/work/project and this repository is checked out at ~/work/composer-diff, run:

cd ~/work/project
~/work/composer-diff

Note that I'm using the composer-diff script rather than calling composer diff in this case: https://github.com/IonBazan/composer-diff/blob/main/composer-diff
This will compare current files agains HEAD but you can also compare files in any locations using --base and --target options.

@darthsteven
Copy link
Contributor Author

Thanks for running the code coverage reports and letting the tests run here etc.

I guess I don't exactly have a good view on all the different possibilities of what the composer.lock file would look like. I've got plenty of Drupal projects to play around with, but tbh. they are all in a good state!

I was testing on a real-life project as you describe. The bit I couldn't get working was xdebug and composer and being able to run the unit tests on my machine rather than via github actions, I'll have another go, and look to get 100% test coverage too.

src/Url/DrupalGenerator.php Outdated Show resolved Hide resolved
src/Url/DrupalGenerator.php Outdated Show resolved Hide resolved
src/Url/DrupalGenerator.php Outdated Show resolved Hide resolved
@IonBazan
Copy link
Owner

@darthsteven for some reason, I'm unable to push to your fork so I pushed some changes into https://github.com/IonBazan/composer-diff/tree/support-drupalorg-packages - kindly update your branch with my changes and let me know if it looks good. I added some more tests to hit 100% coverage and did a couple of fixes.

@darthsteven
Copy link
Contributor Author

@IonBazan Well, those tests are passing at least. I can test with a few composer.lock files/projects I have too, will try to do that soon.

@IonBazan
Copy link
Owner

IonBazan commented Apr 26, 2024

Thanks, I think we should be good to go now. You can continue to test it with your projects. Should any issue occur, we can fix that in a separate PR 👍🏻 Feel free to raise issues or PRs.

Thank you again for your contribution - #24 was already starting to collect dust so I'm glad it's now fixed.

@IonBazan IonBazan merged commit 3926de2 into IonBazan:main Apr 26, 2024
18 checks passed
@IonBazan IonBazan added the enhancement New feature or request label Apr 26, 2024
@darthsteven
Copy link
Contributor Author

Thanks for making the code 100 times better and merging this!

Now we just need a new release so that the github action will start using it...hint hint.

@IonBazan
Copy link
Owner

Released as https://github.com/IonBazan/composer-diff/releases/tag/v1.9.0

@lpeabody
Copy link

Thanks for this, this will make code review so much less tedious :)

@davereid
Copy link
Contributor

Yay, thank you for helping push this along the finish line!

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

Successfully merging this pull request may close these issues.

Support Drupal packages with links and diffs
5 participants