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

remove malfunctions #111

Merged
merged 2 commits into from
Mar 29, 2024
Merged

remove malfunctions #111

merged 2 commits into from
Mar 29, 2024

Conversation

s-ohnishi
Copy link
Contributor

I forgot to do some of the multiple changes.
I also removed unnecessary comments and aligned quotes.

@s-ohnishi
remove malfunctions
7e6e365 
Additional correction because there was a change omission
@codecov
Copy link

codecov bot commented Jul 20, 2023 •
edited
Loading

Codecov Report

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

Project coverage is 5.06%. Comparing base (28b66fd) to head (199f592).

Files Patch % Lines
src/Controllers/CompaniesController.php 0.00% 2 Missing ⚠️
src/Controllers/ProductsController.php 0.00% 2 Missing ⚠️
src/Controllers/ProducttypesController.php 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             master    #111   +/-   ##
========================================
  Coverage      5.06%   5.06%           
  Complexity      158     158           
========================================
  Files            31      31           
  Lines           948     948           
========================================
  Hits             48      48           
  Misses          900     900

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

@s-ohnishi
Copy link
Contributor Author

Added line #L52 was not covered by tests

I have multiple alerts like this, what should I do?

codecov/patch Failing after 1s — 0.00% of diff hit (target 5.06%)

Did I do something wrong?
Should I make a patch and upload it?

@s-ohnishi
Copy link
Contributor Author

I'm preparing the next change (Bootstrap update), but I can't move forward if this is stuck.

aircodepl
aircodepl approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@aircodepl aircodepl left a comment

Choose a reason for hiding this comment

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

I would only give these arrays some space for better readability
array('di'=>null)
→
array('di' => null)

You can install PHP CodeSniffer to your IDE for these.

PM or mention me to approve next PR.

@aircodepl
Copy link
Contributor

@Jeckerson please approve this PR, thanks.

niden
niden requested changes Mar 28, 2024
View reviewed changes
src/Controllers/CompaniesController.php Outdated
@@ -49,7 +49,7 @@ public function searchAction(): void
$this->request->getPost()
);

$this->persistent->searchParams = $query->getParams();
$this->persistent->searchParams = array('di'=>null) + $query->getParams();
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 use the newer syntax (short) for the array please and also run it through codesniffer (spaces needed before and after =>)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have corrected it as requested.

src/Controllers/ProducttypesController.php Outdated
@@ -54,7 +54,7 @@ public function searchAction(): void
$this->request->getPost()
);

$this->persistent->searchParams = $query->getParams();
$this->persistent->searchParams = array('di'=>null) + $query->getParams();
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have also modified this as per your request.
ProductsController also had the same part, so I modified that as well.

@niden
Copy link
Member

niden commented Mar 28, 2024

Added line #L52 was not covered by tests

I have multiple alerts like this, what should I do?

codecov/patch Failing after 1s — 0.00% of diff hit (target 5.06%)

Did I do something wrong? Should I make a patch and upload it?

Don't worry about that.

@s-ohnishi
remove malfunctions more fix
199f592 
Responding to change requests
@s-ohnishi s-ohnishi requested a review from niden March 29, 2024 13:41
@niden niden merged commit 2623b07 into phalcon:master Mar 29, 2024
4 of 5 checks passed
@niden
Copy link
Member

niden commented Mar 29, 2024

Thank you @s-ohnishi

@s-ohnishi
Copy link
Contributor Author

I'm the one who should be thanking you.
Now I can move on.

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

@aircodepl aircodepl aircodepl approved these changes

@niden niden niden approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@s-ohnishi @aircodepl @niden