Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Shreya Shetty created an issue. See original summary.

Shreya Shetty’s picture

Status: Active » Needs review
FileSize
449 bytes
legovaer’s picture

Status: Needs review » Needs work

Hi,

Thanks for your work on this. Here are some suggestions:

  • Use https instead of http for the homepage URL
  • It would be good to add the maintainers of this module to the authors section
  • We should add "source" to the support section
Shreya Shetty’s picture

Assigned: Unassigned » Shreya Shetty
Shreya Shetty’s picture

Status: Needs work » Needs review
FileSize
827 bytes

Thank You for reviewing the patch . I have made some improvements .

jonathan1055’s picture

Title: Composer file should be added to the module » Add composer.json file for Scheduler
FileSize
1.1 KB
1.19 KB

Hi Shreya Shetty,
Thank you for doing this, and thanks legovaer for the review. I took a look at the composer.json instructions and examples and added the user's on-screen name and user page.

I'm not sure about the standard for including non-active maintainers - Eric looked after Scheduler at 5.x & 6.x but has not contributed for three years. Also he was not the founder of this module. We have a long history - the first commits were way back in 2003, by none other than Dries Buytaert, Gábor Hojtsy and Moshe Weitzman themselves!

How do we test this new file?

Shreya Shetty’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks fine .

jonathan1055’s picture

Assigned: Shreya Shetty » Unassigned
Status: Reviewed & tested by the community » Fixed

OK, I have committed the latest version, thanks Shreya.
I would still like to hear legovaer's view on testing/checking this file.

legovaer’s picture

We shouldn't add an automated test for this as this can be tested very easy by executing:

$ composer validate modules/contrib/scheduler/composer.json 
composer.json is valid

I've been looking into automating the test, but then we would need custom libraries defined in our composer.json file. Currently, DrupalCI does not support installation of these libraries during the testing phase. As soon as we have our module working with 8.1.x, it will become easier and we can maybe think about automating this.

jonathan1055’s picture

No, I did not mean we wanted an automated test, but I thought there should be some script or command to use for checking the syntax. Thanks for the info. I have not installed composer, so I guess I should go and get it and learn about it.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.