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

[PSR4] Add UniteFileAndClassNameRector #1170

Closed
wants to merge 1 commit into from
Closed

Conversation

TomasVotruba
Copy link
Member

Closes #1154

@TomasVotruba
Copy link
Member Author

@JanMikes Ready for review :)

@TomasVotruba TomasVotruba force-pushed the psr-name branch 3 times, most recently from 46fb837 to fb3cd96 Compare March 9, 2019 20:46
CODE_SAMPLE
,
<<<'CODE_SAMPLE'
// file: SomeClass.php
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the code and test case, it renames the file not the class. This example shows that the file name stays the same but the class inside gets renamed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add an option to let the user decide in which direction they want to sync the name? From file to class or from class to file? I could imagine that this is a very hard decision though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to automate this for user. Any case for "asking" should be converted to algorithm or not handled by Rector at all but by a sniff.

I thought about this and there should be some function that detects which of file/class is typoed. Any idea for typo-detecting PHP package?

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we'd add a spell checker, the user would still have to configure which language he used for his project, which would then again be a case of "asking" I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Better done than perfect. Got any idea about the package?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you think that's the best option, got for it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi guys! I agree that it would be awesome to let user choose, what way he wants to use it.

  1. Fix filename
  2. Fix classname and all usages in app

When i say let user choose there might be option/parameter in rector's config, maybe with default value? No prompts or ask dialogs :-).

Copy link
Member Author

Choose a reason for hiding this comment

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

Rector's idea is not to bother user at all. Anything requiring user's attention is better done in PHPStorm, where you prefer control with slow work.

Rector should handle typos for users, like SomeeeClass in SomeClass.php. Any user-opinionated moving is not Rector's job.

It would be better to add more real-life tests, so I know what you need this for :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Real life example:
SomeClassPresenter + SomeClass.php - discussion with team, agreed to rename SomeClass.php -> SomeClassPresenter.php this will be most common use case for us now, but i can imagine in some cases we want to rename class instead of file. Maybe create 2 rectors? One for renaming files, different for renaming classes? 😄

Copy link
Member Author

@TomasVotruba TomasVotruba Mar 11, 2019

Choose a reason for hiding this comment

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

I'd do these manually, since it needs team energy anyway.

Rector would only add work here instead. Not a way to go :)

Copy link
Contributor

@JanMikes JanMikes left a comment

Choose a reason for hiding this comment

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

Can this fix namespaces as well?

I found them to be broken in project i am working now as well.

Real life example:
libs/klarka/PageTimeApc and class is Klarka\libs\PageTimeApc

With directory registered as PSR-4 root, this would become Klarka\PageTimeApc

If this was supported i would be able to fix whole project's PSR-4 and replace Nette\RobotLoader instantly 😄

@nschoellhorn
Copy link
Contributor

I think that would be the case for a different rector, right @TomasVotruba ? I would go only for renaming the namespace instead of renaming the folder in that case.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Mar 11, 2019

I think that would be the case for a different rector, right @TomasVotruba ? I would go only for renaming the namespace instead of renaming the folder in that case.

Yep, different issue.

@TomasVotruba
Copy link
Member Author

Closing as too vague to automate. This is better done manually, with human decission.

@nschoellhorn
Copy link
Contributor

Just to be sure: you don't want to follow the spell checker approach anymore? I've already started the development of a spell checking library that doesn't require additional binaries. If not needed here, I'd push back further development of it a little bit though.

@TomasVotruba
Copy link
Member Author

Nono, sorry if you invested some time it.

These opinionated filesystem changes would often definitely go wrong and annoy people. That's not feeling Rector should bring up.

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

3 participants