Skip to content

refactor: convert to namespaces#245

Merged
johnhooks merged 9 commits into
WordPress:developfrom
johnhooks:fix/naming
Apr 11, 2023
Merged

refactor: convert to namespaces#245
johnhooks merged 9 commits into
WordPress:developfrom
johnhooks:fix/naming

Conversation

@johnhooks
Copy link
Copy Markdown
Collaborator

@johnhooks johnhooks commented Apr 6, 2023

What?

Implement naming convention to discussed in Issue #197.

Why?

There is currently a few differently systems of naming across the project and it should be consistant.

How?

  • Remove WP_Notify class prefix
  • Remove wp-notify file prefix
  • Convert to namespace WP\Notifications

Also cleans up some misaligned PHPDoc comments.

Copy link
Copy Markdown
Collaborator Author

@johnhooks johnhooks left a comment

Choose a reason for hiding this comment

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

This is a first step to fix naming in the PHP code. The only code removed are the serialization tests. They are problematic. The previous system of serializing class instances into JSON that is initialized based on class names embedded into the JSON won't fit well with standard WordPress rest-api schemas.

Comment thread wp-feature-notifications.php
@johnhooks johnhooks changed the title Fix/naming refactor: convert to namespaces Apr 6, 2023
@johnhooks johnhooks requested review from Sephsekla and erikyo April 6, 2023 14:29
Copy link
Copy Markdown
Collaborator

@Sephsekla Sephsekla left a comment

Choose a reason for hiding this comment

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

In general there's a lot that needs doing still in terms of documentation etc - there are a lot of classes etc which need commenting (and as discussed, maybe some things we can remove).

Added one thought around namespace imports, let me know what you think.

Comment thread includes/class-factory.php Outdated
Comment thread wp-feature-notifications.php
@johnhooks
Copy link
Copy Markdown
Collaborator Author

johnhooks commented Apr 7, 2023

@Sephsekla I agree, this is just a first step. How would you like to work through the whole process?

I would prefer doing it in stages so each PR is simple in purpose and easier to review. This one is rather large, but I think its narrow scope makes it easy to review and understand.

@johnhooks johnhooks requested a review from Sephsekla April 8, 2023 03:09
Copy link
Copy Markdown
Collaborator Author

@johnhooks johnhooks left a comment

Choose a reason for hiding this comment

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

I checked everything and this is ready for review.

Comment thread includes/demo.php
@johnhooks johnhooks removed the request for review from Sephsekla April 11, 2023 20:44
Copy link
Copy Markdown
Collaborator

@erikyo erikyo left a comment

Choose a reason for hiding this comment

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

I have searched around all files but there is no more trace of wp-notify! Good job @johnhooks and thanks, now we have all the files consolidated 🚀

@johnhooks johnhooks merged commit 5936a15 into WordPress:develop Apr 11, 2023
@johnhooks johnhooks deleted the fix/naming branch April 11, 2023 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants