Box-Of-Hats / Bem-VSCode-Extension

A VSCode extension for helping with inserting BEM (Block-Element-Modifier) classes.
BSD 3-Clause "New" or "Revised" License
29 stars 5 forks source link

Using PHP code in class shows Diagnostic Warnings #10

Closed zeshanshani closed 4 years ago

zeshanshani commented 4 years ago

Hey mate,

Here's the issue: https://s.zeshanahmed.com/a2_234B5E2B.png

So if you use PHP code like <?php it will still consider it as a simple string. I hardly think that anyone would use special characters as class names so I think it's nice idea to exclude < > % { } &... from the testing regex.

Here's another screenshot: https://s.zeshanahmed.com/a_234B5DEF.png

Thanks, Zeshan

Box-Of-Hats commented 4 years ago

Sounds like a straightforward fix. I'll take a look into this later today.

Thanks for the suggestion

Box-Of-Hats commented 4 years ago

After looking at this, it's a bit of a more complicated issue.

If for example, someone is using kebab-casing for classes but they include a php variable $header_classes or a function tfnew_join_classes( ... ), these will both throw errors because they contain _ , which is a snake-case character.

It's possible to add special characters such as $<> etc to the classname matching regex but the cases where _ are present are more complex

Box-Of-Hats commented 4 years ago

I've added some new code that will ignore any characters inside of a <?php ?> string.

From my testing, this appears to have fixed this issue but it will need further testing with a real PHP codebase.

Let me know if this is now working for you :)

zeshanshani commented 4 years ago

@Box-Of-Hats thanks a lot for working on it. I'm afraid I still see the same issue with 0.8.0 version installed: https://s.zeshanahmed.com/Cursor_234D3842.png

Box-Of-Hats commented 4 years ago

Darn! I feel like we're very close to finishing this one.

For me, the line <li class="nav-item portal-nav__item<?php echo tfnew_is_page( '/portal/donations/' ) ? 'active : ''; ?>"> doesn't cause any diagnostics issues.

Due to the way the classname errors are calculated, I'm thinking that there's an issue somewhere else in the file that's being picked up.

Are there any other diagnostic issues happening in that same file?

zeshanshani commented 4 years ago

@Box-Of-Hats No, the line is only showing the BEM diagnostic warnings. Not sure what could be wrong but I think it's still not considering <?php ?> tags.

Can I do anything else that can help to narrow down the issue?

Box-Of-Hats commented 4 years ago

What is the smallest amount of code you can get so that the issue is reported?

E.g does that line on its own cause any issues, and if it doesn't, what else needs to be in the file to get it to happen.

(This is so I can replicate the issue on my machine. I know you're getting it in a full file but I dont expect you to share a complete file from your codebase with me!)

untainsYD commented 4 years ago

Warnings with the inline php tag still causes invalid parsing but it a little bit another problem then has been described in that issue.

Description

Terminal problems tab shows a lot of warnings because it triggers on php inline tag in non-class attribute. The class is written without error but this is a problem with php inline tags in other attributes like href, src, alt etc. which are getting parsed. But they should not.

Example of code

<a class="navbar-brand" href="<?php echo esc_url( home_url() ); ?>">

note: after removing <?php ... ?> warning is gone

Warning sample

BEM - Class names must be in kebab case

Plugin settings

VS Code setting.json file

"bemHelper.showDepthWarnings": true,
"bemHelper.classNameCase": "kebab",
"bemHelper.responsiveLinting": true,
Box-Of-Hats commented 4 years ago

Thanks for the detailed bug report!

I've made some changes to the way the extension handles ignore strings, specifically for PHP in version 1.1.2.

This should fix this issue. Let me know if you're still having any issues

version 1.1.2

untainsYD commented 4 years ago

I have tried to reinstall extension from VS Code Extension Market by clicking Install button and after redirection, to my VS Code app on my Windows 7 machine, I see only 0.8.0 version of the plugin. Also, if I select Install Another Version in VS Code Application (Ctrl + Shift + X --> right click on your's extension --> Install another version), I see that last version is 0.8.0.

Extension ID: box-of-hats.bemhelper


Note: on the Linux machine - Debian 10 Buster all is fine and I have 1.1.3 v.

Box-Of-Hats commented 4 years ago

It sounds like you may be running an older version of VSCode. There is a minimum requirement for the extension that I may be able to adjust so that the extension is available on older versions of VSCode.

I'll do some investigation

Box-Of-Hats commented 4 years ago

I've reduced the minimum VSCode version back to what it was in version 0.8.0 of the extension (1.32.0).

Are you now able to see 1.1.4?

@untains-yd

untainsYD commented 4 years ago

Yes, all is fine!

Box-Of-Hats commented 4 years ago

Great, thanks for the update :)