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

Add status and method in calendar event #300

Open
wants to merge 4 commits into
base: 2.x
Choose a base branch
from

Conversation

giuseppenucifora
Copy link

  • Add status property in Event
  • Add method property in Event

@markuspoerschke
Copy link
Owner

markuspoerschke commented Jul 6, 2021

Hey Giuseppe,

thanks for your contribution. There are some problem that needs to be solved before I can merge this pull request:

  • The CI-Pipeline is failing. Please check the logs and resolve the issues. (You can fix the code style running make fix and executing the tests locally using make test.)
  • Please update the documentation as well.
  • A new entry to the changelog is needed to inform other developers about this new feature.

src/Domain/Entity/Event.php Outdated Show resolved Hide resolved
src/Presentation/Factory/EventFactory.php Outdated Show resolved Hide resolved
@@ -111,7 +113,7 @@ protected function getProperties(Event $event): Generator
protected function getComponents(Event $event): Generator
{
yield from array_map(
fn (Alarm $alarm) => $this->alarmFactory->createComponent($alarm),
Copy link
Owner

Choose a reason for hiding this comment

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

The code style was changed here. That is one of the reasons, why the CI pipeline fails.

You can fix the code style by running make fix locally.

@giuseppenucifora
Copy link
Author

@markuspoerschke as you rightly suggested, I refactor the code by adding two enum for method and status but now launching test , the phpmd test gave this error :

phpmd ./src text rulesets.xml /Volumes/External/Progetti/GiuseppeNucifora/GIT/PHP/iCal/src/Presentation/Factory/EventFactory.php:74 The method getProperties() has a Cyclomatic Complexity of 10. The configured cyclomatic complexity threshold is 10. /Volumes/External/Progetti/GiuseppeNucifora/GIT/PHP/iCal/src/Presentation/Factory/EventFactory.php:74 The method getProperties() has an NPath complexity of 512. The configured NPath complexity threshold is 200. make: *** [test-phpmd] Error 2

can you help me to solve? Scuse me, but unfortunately i don't know this. :)

- Add method property in Event
- Update Unit test to test new properties
@giuseppenucifora giuseppenucifora force-pushed the feature/event-status-and-method branch from f1673ae to ba81b43 Compare July 6, 2021 10:40
@markuspoerschke
Copy link
Owner

phpmd ./src text rulesets.xml /Volumes/External/Progetti/GiuseppeNucifora/GIT/PHP/iCal/src/Presentation/Factory/EventFactory.php:74 The method getProperties() has a Cyclomatic Complexity of 10. The configured cyclomatic complexity threshold is 10. /Volumes/External/Progetti/GiuseppeNucifora/GIT/PHP/iCal/src/Presentation/Factory/EventFactory.php:74 The method getProperties() has an NPath complexity of 512. The configured NPath complexity threshold is 200. make: *** [test-phpmd] Error 2

This is a violated rule from PHPMD. Since the Factory class will assemble a lot of properties, I think we can ignore the error message. You could add a @SuppressWarnings("NPathComplexity") to the method. See https://phpmd.org/documentation/suppress-warnings.html

src/Domain/Enum/MethodType.php Outdated Show resolved Hide resolved
src/Domain/Enum/StatusType.php Outdated Show resolved Hide resolved
markuspoerschke and others added 2 commits July 28, 2021 15:45
…nt (markuspoerschke#302)

* remove PHP-CS-Cache v3 with make clean
* dont use phony target for composer dependencies:
  don't install composer dependencies if already installed
  this will speedup `make test` locally
* dont install node_modules if present when `make fix-prettier` is called
Change MethodType enum tu Method
Add @SuppressWarnings("PHPMD.NPathComplexity") in getProperties function of EventFactory class
Add @SuppressWarnings("PHPMD.CyclomaticComplexity") in getProperties function of EventFactory class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants