Skip to content

Fix to strict rules and exclude pattern uses #13

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

Conversation

elzekool
Copy link

@elzekool elzekool commented Jun 6, 2024

  • Fix Exclude pattern, the patterns did not include the dot of the extension allowing matching with the pathname (which for example in DDEV will match with html in /var/www/html)

  • Included more rules to exclude for (p)html and xml files. Indentation, line-length are very hard to fix consistently and prevents usage of component libraries (as for example with Hyvä)

Copy link

@fredden fredden left a comment

Choose a reason for hiding this comment

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

I would prefer to see these two changes as separate pull requests. One seems like a bugfix (properly escape the . in the pattern), and another a policy change (apply rules to files or not).

Copy link
Member

@igorwulff igorwulff left a comment

Choose a reason for hiding this comment

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

Looks good. I'm not disagreeing with Dan with the preferable separate pr's, but prefer to remain pragmatic in this case and push this change forward.

* Fix Exclude pattern, the patterns did not include
  the dot of the extension allowing matching with
  the pathname (which for example in DDEV will
  match with `html` in `/var/www/html`)

* Included more rules to exclude for (p)html and xml
  files. Indentation, line-length are very hard to
  fix consistently and prevents usage of component
  libraries (as for example with Hyvä)
@elzekool elzekool force-pushed the bugfix/fix-to-strict-rules-and-exclude-pattern-uses branch from 224ea9f to cb0454b Compare June 6, 2024 15:32
@elzekool
Copy link
Author

elzekool commented Jun 6, 2024

Hi @fredden I tested and fixed your suggestion about the leading *. and removed it.

@elzekool
Copy link
Author

elzekool commented Jun 6, 2024

@fredden @igorwulff I kind of agree with your point on the separate PR's especially from a library/package standpoint. But from an impact standpoint they are targeting the same issue, to get the issue fixed the first PR then needed to be merged (and tagged) before I could/can create the next PR while at the moment my project is having issues with the package and other teams are just making work-a-rounds to skip checks which they should not.

@igorwulff
Copy link
Member

Lets wait until tomorrow afternoon, and give some time for the others to have checked this PR. If there are no blockers we can merge it.

@igorwulff igorwulff merged commit 2355959 into YouweGit:master Jun 10, 2024
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.

4 participants