Skip to content

refactor: renamed UserDto in core project to AbstractBaseUserDto, renamed AppUserDto in subprojects to UserDto #581

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cholakov11
Copy link
Contributor

Renamed UserDto class to AbstractBaseUserDto

  • Although the AbstractBaseUserDto class is not defined as an abstract class, I believe it should be named as abstract. This is because in our context the dto class is used to transport data of the AbstractBaseUser class. Naming the dto class also abstract should avoid any misinterpretations that this dto refers to a class other than the AbstractBaseUser.

@JelmenGuhlke
Copy link
Contributor

JelmenGuhlke commented Oct 2, 2024

@cholakov11 so your suggestion is to use the name AbstractBaseUserDto for the DTO in this PR?

@JelmenGuhlke JelmenGuhlke self-requested a review October 2, 2024 09:26
@cholakov11
Copy link
Contributor Author

@JelmenGuhlke , yes. I have also implemented it this way.

Copy link
Contributor

@JelmenGuhlke JelmenGuhlke left a comment

Choose a reason for hiding this comment

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

Changes LGTM -> please rebase the branch ontop of main


- `UserDto` class has been renamed to `AbstractBaseUserDto`. Each call to the `UserDto` class should be changed to call
`AbstractBaseUserDto`.
- `AppUserDto` classes in the subprojects `identity-model`, `sequence-model`, `uuid-model` have been renamed to
Copy link
Contributor

Choose a reason for hiding this comment

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

After a rebase on main, these changes are not required anymore, since the packages are deleted

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.

2 participants