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

civilint - Call "civistrings" and look for warnings #784

Closed
wants to merge 1 commit into from

Conversation

totten
Copy link
Member

@totten totten commented Jun 21, 2023

@mlutfy - I was reading this comment and took a quick stab at updating civilint. In principle, this would cause the style-checks to fail if a modified file has any warnings from civistrings.

There are two things I'm not sure about here:

  1. The specific error message would only be on the console. (Takes a little more work to bubble-up to the report in Jenkins.) But that's livable (and maybe fixable).
  2. It may be a little too zealous? Like, it would enforce style-rules on pre-existing things in all branches (e.g. current patch went into 5.64-dev; so 5.63-rc and 5.62-stable still have more messy bits). And there are a few oddballs where it's intentionally deviant (e.g. Civi/Angular/Manager.php getTranslatedStrings()).

@mlutfy
Copy link
Member

mlutfy commented Jun 21, 2023

@totten 🎉 this would be great

For (1), would it still cause the build to fail? (so that we can investigate, see the error). If yes, sounds OK to me. We can tweak later.

For (2), maybe - there are legitimate situations where someone might do ts($name) - not many, but in some obscure places of core or SearchKit/FB, it happens. Maybe we could have a wrapper ts function that is for those situations?

@totten
Copy link
Member Author

totten commented Jun 22, 2023

For (1), yes it would fail, so it does that job. To make it fail more nicely, I've a separate issue on the backlog (civicrm/civistrings#14).

For (2), I like that idea and opened civicrm/civicrm-core#26595. (Still need something else to deal with the 5.63-vs-5.64 issue -- for that aspect, feels like linter needs to somehow incorporate the branch/version.)

@github-actions
Copy link

This pull request has reached 30 days with no activity, it has been marked as stale.

@github-actions github-actions bot closed this Jul 30, 2023
@mlutfy mlutfy reopened this Jul 30, 2023
@github-actions github-actions bot closed this Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants