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

Check namespaces and class imports for leading backslash #61

Merged
merged 1 commit into from
Sep 17, 2023

Conversation

andrewnicols
Copy link
Contributor

Fixes #20

@codecov
Copy link

codecov bot commented Sep 17, 2023

Codecov Report

Merging #61 (e3dae34) into main (5f76734) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main      #61      +/-   ##
============================================
+ Coverage     95.53%   95.62%   +0.08%     
- Complexity      440      445       +5     
============================================
  Files            19       20       +1     
  Lines          1165     1188      +23     
============================================
+ Hits           1113     1136      +23     
  Misses           52       52              
Files Changed Coverage Δ
...odle/Sniffs/Namespaces/NamespaceStatementSniff.php 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stronk7
Copy link
Member

stronk7 commented Sep 17, 2023

Q: Shouldn't we also add PSR12.Files.ImportStatement.LeadingSlash to get imports also covered?

And little detail, it looks "strange" that our class is named "NoLeadingSlash", shouldn't it be better named "LeadingSlash".

Similar sentiment with the problem name, we are naming it "NamespaceStartsWithSlash", that is redundant ("namespace") and uses different wording. Why not, also for the problem, just use "LeadingSlash".

FYC, ciao :-)

PS, I've seen that there are a few tests failing right now...

@andrewnicols andrewnicols force-pushed the noLeadingBackslash branch 3 times, most recently from 3dc1903 to 7c26cd9 Compare September 17, 2023 09:43
@andrewnicols
Copy link
Contributor Author

Q: Shouldn't we also add PSR12.Files.ImportStatement.LeadingSlash to get imports also covered?
Bah - I originally applied this to the extra ruleset which already has it. Updated to include it/.

And little detail, it looks "strange" that our class is named "NoLeadingSlash", shouldn't it be better named "LeadingSlash".

Similar sentiment with the problem name, we are naming it "NamespaceStartsWithSlash", that is redundant ("namespace") and uses different wording. Why not, also for the problem, just use "LeadingSlash".

If I make this LeadingSlash and name the problem LeadingSlash it becomes moodle.Namespace.LeadingSlash.LeadingSlash which is also redundant.

Rather than that, I've tried to take inspiration from the PSR-12 one and now have:
moodle.Namespace.NamespaceStatement.LeadingSlash

I think the duplication of Namespace in both the Sniff type, and the sniff name is okay because it's a check of the Namespace statement and naming it simply Statement could be confusing - moodle.Namespace.Statement is fine, but Statement on its own may confuse.

Thoughts?

PS, I've seen that there are a few tests failing right now...

phplint was actually picking this up already so I think I've got that now!

Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

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

I like making it NamespaceStatement so we can add later more checks there.

Note: I'm completing the API checks for PHPUnit classes (that already have Namespace checks). Once working I plan to move them from being "phpunit only" to be "whole codebase" ones. Surely then we'll be adding more checks to the NamespaceStatement sniff added here.

@andrewnicols
Copy link
Contributor Author

Note: I'm completing the API checks for PHPUnit classes (that already have Namespace checks). Once working I plan to move them from being "phpunit only" to be "whole codebase" ones. Surely then we'll be adding more checks to the NamespaceStatement sniff added here.

Perfect - that was about what I had in mind too!

@stronk7 stronk7 merged commit 8e1fc14 into moodlehq:main Sep 17, 2023
7 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.

[CONTRIB-7828] Implement checks for namespace/import statements
2 participants