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

Match file name to type name when possible #14159

Merged
merged 4 commits into from Aug 17, 2023

Conversation

MikeAlhayek
Copy link
Member

@jtkech there are more mismatch that I expected. I renamed the filenames to the type when possible. In some cases, I moved the types to their own file.

@jtkech
Copy link
Member

jtkech commented Aug 16, 2023

Yes so many changes, that's why I haven't even tried it, waiting for having more time ;)

What I did in #14151 was more like fixing a "typo" in 3 file names than following the SA1649 rule.

Will be the same for example if we update namespace declarations, and so on.

Maybe we should not be too strict, I don't know, I've no closed opinion.

Maybe need to be discussed in a meeting to adopt the most reasonable compromises.

And for now just let things as they are (easiest compromise).

@MikeAlhayek
Copy link
Member Author

Maybe we should not be too strict, I don't know, I've no closed opinion.

Yes that is why I did not enforce the rule. All the changes in this PR is just to match the file name to the type. We still have some types that do not match filename.

But I think what I included in this PR should be acceptable as it does not impact much except for making the file more consistence.

@jtkech
Copy link
Member

jtkech commented Aug 16, 2023

Okay then, I will review it asap.

Copy link
Member

@jtkech jtkech left a comment

Choose a reason for hiding this comment

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

Can't check every file but LGTM.

Good job!

@MikeAlhayek MikeAlhayek merged commit 6c8323d into OrchardCMS:main Aug 17, 2023
3 checks passed
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