Skip to content

Fixes & refactor #3

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 9 commits into
base: main
Choose a base branch
from
Open

Fixes & refactor #3

wants to merge 9 commits into from

Conversation

daycry
Copy link
Contributor

@daycry daycry commented Oct 16, 2024

No description provided.

daycry added 5 commits May 22, 2024 16:09

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@angelov
Copy link
Owner

angelov commented Oct 18, 2024

Thank you for the contribution @daycry. Can you please provide more info why we need these changes?

angelov added 2 commits March 6, 2025 19:04

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Jordi de la Mano and others added 2 commits March 11, 2025 11:17
@daycry
Copy link
Contributor Author

daycry commented Mar 11, 2025

@daycry
Copy link
Contributor Author

daycry commented Mar 11, 2025

Coding standards checks with composer cs command

@daycry daycry changed the title Fix: Get Name test Fixes & refactor Mar 11, 2025
@angelov
Copy link
Owner

angelov commented Mar 11, 2025

There are too many unrelated changes for a single PR. Please split them into several PRs instead, so we can discuss them separately. Don't forget to add description and examples on why the changes are needed.

Here are few things which I don't think are needed:

  • Additional Composer dependencies, config files, and script aliases
  • Badges in the readme file (already explained in README Info #2)
  • Git hooks - We already have the workflows here
  • License info in each file - There's a license file

@daycry
Copy link
Contributor Author

daycry commented Mar 11, 2025

Sorry, but I can't spend more time updating your package.

You told me we can collaborate and I do. If you install want you can check these changes or you can delete it, but the changes made are very interesting.

@angelov
Copy link
Owner

angelov commented Mar 11, 2025

I asked you to push the changes that you needed here, so you can correct the fact that you had copied the code and removed the references to the original work. I can support adding these changes to this package, but that does not mean you can just throw a bunch of changes in one PR without any additional context. If what is present here does not work for you, you are free to fork it and modify it as you wish, but you have to respect the license.

@daycry
Copy link
Contributor Author

daycry commented Mar 15, 2025

there are many changes in all the files because I have added your signature to the beginning of each file.
I have modified the cs-fixer package to keep it up to the standards, At the code level I have basically modified how you retrieve the method or class to get the attribute.

If you are scared to see so many changes, I will divide them.

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.

None yet

2 participants