Skip to content

Expand UserList entity interface#3660

Merged
demiankatz merged 37 commits intovufind-org:devfrom
padmasreegade:user-list-factory-entity-interface
May 23, 2024
Merged

Expand UserList entity interface#3660
demiankatz merged 37 commits intovufind-org:devfrom
padmasreegade:user-list-factory-entity-interface

Conversation

@padmasreegade
Copy link
Contributor

Implemented setter and getter methods for UserList.

@padmasreegade padmasreegade marked this pull request as draft May 13, 2024 18:41
@demiankatz demiankatz changed the title User list factory entity interface Expand UserList entity interface May 13, 2024
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

@padmasreegade, you can run composer fix from the $VUFIND_HOME directory to correct most of the remaining style issues automatically. Once you've done that, please let me know and I'll take a closer look at the changes!

@demiankatz
Copy link
Member

Looks like there are now some phpstan observations that require manual fixing. Let me know if you need any more help; otherwise, I'll just wait until you request a review from me. :-)

@padmasreegade
Copy link
Contributor Author

padmasreegade commented May 13, 2024

Looks like there are now some phpstan observations that require manual fixing. Let me know if you need any more help; otherwise, I'll just wait until you request a review from me. :-)

Thank you for offering, Demian! I'm working on them, will let you know if I need something 😄

@padmasreegade padmasreegade marked this pull request as ready for review May 13, 2024 20:41
@padmasreegade padmasreegade requested a review from demiankatz May 13, 2024 20:41
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @padmasreegade! See below for a few comments and suggestions.

@padmasreegade padmasreegade requested a review from demiankatz May 14, 2024 15:52
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @padmasreegade -- a few more issues/suggestions.

Also, a broader matter that was just raised in #3638: a lot of this code has inconsistent punctuation in the comments. Some method descriptions end with periods and others do not. It would probably be best to make this consistent one way or the other. If you have a preference, feel free to go ahead and edit accordingly. If you're not sure, you can watch the conversation on #3638 and see where it leads.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Progress is looking good! I realize you haven't requested another review, so apologies if any of these suggestions are premature. I have a meeting coming up soon, so I thought I'd leave some feedback before I get busy! :-)

@padmasreegade padmasreegade requested a review from demiankatz May 14, 2024 18:29
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @padmasreegade -- I think this is nearly done; I just have a few minor remaining suggestions.

I suspect that several of the comments I made on the row class will require matching changes to the entity interface; please double-check those as you go -- I thought it better not to leave double-comments. :-)

@padmasreegade padmasreegade requested a review from demiankatz May 23, 2024 17:47
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @padmasreegade -- this is ready to merge!

@demiankatz demiankatz merged commit 30571d7 into vufind-org:dev May 23, 2024
@demiankatz demiankatz added this to the 10.0 milestone May 23, 2024
@demiankatz demiankatz added the architecture pull requests that involve significant refactoring / architectural changes label May 23, 2024
@padmasreegade padmasreegade deleted the user-list-factory-entity-interface branch May 23, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

architecture pull requests that involve significant refactoring / architectural changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants