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

Refactors "strpos" calls in /apps/workflowengine #38604

Merged

Conversation

fsamapoor
Copy link
Member

Summary

Following #38261 and #38260, I have replaced strpos calls in /apps/workflowengine namespace as well.

@szaimen szaimen added 3. to review Waiting for reviews technical debt labels Jun 2, 2023
@szaimen szaimen added this to the Nextcloud 28 milestone Jun 2, 2023
@szaimen szaimen requested review from a team, ArtificialOwl, icewind1991 and come-nc and removed request for a team June 2, 2023 12:12
@come-nc come-nc force-pushed the replace_strpos_calls_in_workflowengine_app branch from 1529669 to 4831157 Compare June 5, 2023 09:06
@fsamapoor fsamapoor force-pushed the replace_strpos_calls_in_workflowengine_app branch from c1b33fb to c4385ae Compare July 3, 2023 09:37
fsamapoor and others added 2 commits July 3, 2023 13:13
…ability.

Signed-off-by: Faraz Samapoor <f.samapoor@gmail.com>
Signed-off-by: Faraz Samapoor <fsa@adlas.at>
Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
Signed-off-by: Faraz Samapoor <f.samapoor@gmail.com>
Signed-off-by: Faraz Samapoor <fsa@adlas.at>
@fsamapoor fsamapoor force-pushed the replace_strpos_calls_in_workflowengine_app branch from c4385ae to 9d15e6e Compare July 3, 2023 09:43
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@fsamapoor
Copy link
Member Author

fsamapoor commented Aug 7, 2023

Hello @come-nc,

Sorry to bother you. I understand that reviewing PRs can take some time, but sometimes my PRs do not get merged after being approved and some of them have more than two approvals. Should I merge them myself once all the required checks have passed? Are there any considerations in place to merge PRs?

Thank you for your assistance.

@come-nc come-nc merged commit bfe42de into nextcloud:master Aug 7, 2023
@come-nc
Copy link
Contributor

come-nc commented Aug 7, 2023

Hello @come-nc,

Sorry to bother you. I understand that reviewing PRs can take some time, but sometimes my PRs do not get merged after being approved and some of them have more than two approvals. Should I merge them myself once all the required checks have passed? Are there any considerations in place to merge PRs?

Thank you for your assistance.

Hey, do not hesitate to notify us if it goes unnoticed for too long. For PRs on master at least.

(But according to psalm on this one ?? is used where ?: should have been since getPathInfo can return false. But then ?: I suppose would eat strings like '0'. Not sure what the clean thing is, also I did not check what getPathInfo actually returns.)

@fsamapoor
Copy link
Member Author

Hello @come-nc,
Sorry to bother you. I understand that reviewing PRs can take some time, but sometimes my PRs do not get merged after being approved and some of them have more than two approvals. Should I merge them myself once all the required checks have passed? Are there any considerations in place to merge PRs?
Thank you for your assistance.

Hey, do not hesitate to notify us if it goes unnoticed for too long. For PRs on master at least.

(But according to psalm on this one ?? is used where ?: should have been since getPathInfo can return false. But then ?: I suppose would eat strings like '0'. Not sure what the clean thing is, also I did not check what getPathInfo actually returns.)

Sure. Thanks.

I applied these changes based on your suggestion in the review:
#38604 (comment)

I just checked the codebase. The getPathInfo method in the Request class returns string, but the IRequest interface has the string|false annotation for the getPathInfo method. So either way, the code should work fine with or without any of the ?? or ?: operators. Making Psalm happy would need to update the getPathInfo method annotations in the Request class. Let me know if I should revert the commit that adds the ?? operator to the mix.

@come-nc
Copy link
Contributor

come-nc commented Aug 7, 2023

I applied these changes based on your suggestion in the review: #38604 (comment)

I just checked the codebase. The getPathInfo method in the Request class returns string, but the IRequest interface has the string|false annotation for the getPathInfo method. So either way, the code should work fine with or without any of the ?? or ?: operators. Making Psalm happy would need to update the getPathInfo method annotations in the Request class. Let me know if I should revert the commit that adds the ?? operator to the mix.

Let’s leave it like that for now.

@fsamapoor fsamapoor deleted the replace_strpos_calls_in_workflowengine_app branch March 8, 2024 07:42
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.

4 participants