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

Remove superfluous words from the English allow list #413

Merged
merged 9 commits into from
Oct 18, 2022

Conversation

fritzmg
Copy link
Collaborator

@fritzmg fritzmg commented Feb 26, 2022

This cleans up the English allow list a bit.

@fritzmg fritzmg enabled auto-merge (squash) February 26, 2022 15:38
@Total-Reality
Copy link
Contributor

Total-Reality commented Feb 26, 2022

  1. Couldn't the following words also be removed in this context?:
  • Assistant
  • Avatar
  • base
  • Card
  • Container
  • etc
  • login
  • Notification
  • notification
  • Panel
  • scalable
  • Social
  • Spam
  • staging
  • Ticker
  1. The following words are misspelled and could be removed also (but it is possible that this words are part of the name of extensions):
  • Api (already exists in default.txt as API)
  • infos (info is correct)
  • navigations (navigation is correct)
  • Socialmedia (Social media is correct)
  • youtube (already exists in default.txt as YouTube)
  1. The following words should be moved to default.txt (most of them exist in de.yml + en.yml btw.)
  • AntiSpam
  • Bootstrapper
  • Cookiebar
  • EasyThemes
  • Filemanager
  • Geocode
  • Geodata
  • Jobposting
  • LinkedIn
  • Mega
  • Merger
  • Picker
  • Plenta
  • Slider
  • Social
  • Softleister
  • Teaserbox
  • tmp

@fritzmg
Copy link
Collaborator Author

fritzmg commented Feb 26, 2022

@Total-Reality feel free to create a separate PR for that.

@Total-Reality
Copy link
Contributor

@Total-Reality feel free to create a separate PR for that.

Okay, thank you. I will create one as soon as the lint action (bug?) is fixed #414

xchs
xchs previously approved these changes Mar 21, 2022
@xchs
Copy link
Collaborator

xchs commented Mar 21, 2022

What's the problem with the linter?

@fritzmg
Copy link
Collaborator Author

fritzmg commented Mar 21, 2022

Unknown, see #414

@Total-Reality
Copy link
Contributor

Is it possible that this problem occurs because you removed words that are not in the dictionary but used by at least one extension?

@fritzmg
Copy link
Collaborator Author

fritzmg commented Mar 22, 2022

That would be identified by the action itself. But the action is not running for some reason.

@xchs xchs disabled auto-merge April 2, 2022 17:21
@fritzmg fritzmg enabled auto-merge (squash) June 20, 2022 11:34
@aschempp
Copy link
Member

aschempp commented Sep 7, 2022

@fritzmg are you going to finish this PR? 😊

@fritzmg
Copy link
Collaborator Author

fritzmg commented Sep 7, 2022

It was already finished, I just could not figure out what the problem with the linter was. I will update the PR again.

@fritzmg
Copy link
Collaborator Author

fritzmg commented Sep 7, 2022

@aschempp since the linter does not run automatically I unfortunately have currently no way to test the changes. Someone would need to test it in their local environment.

@aschempp
Copy link
Member

Someone would need to test it in their local environment.

Could that someone be you? 🙃
What would happen if a word is missing in the dictionary? Would the next PR fail or would indexing fail forever?

@fritzmg
Copy link
Collaborator Author

fritzmg commented Sep 16, 2022

Could that someone be you? 🙃

I could, but I would need to update my WSL setup. Someone already on a Unix system might have an easier time though.

What would happen if a word is missing in the dictionary? Would the next PR fail or would indexing fail forever?

I don't know, didn't you build the Indexing? ;)
My assumption is that the linter is only used for PRs.

@aschempp
Copy link
Member

@fritzmg I would suggest to merge this as-is and check if it works. There's apparently a cronjob to check the files every month (https://github.com/contao/package-metadata/actions/runs/3219322454/jobs/5264551105) but not sure if that is taken care of anyway 🙈

@fritzmg
Copy link
Collaborator Author

fritzmg commented Oct 14, 2022

Yes, I think this can be merged as is. If a word is missing, it will show up in subsequent meta data PRs anyway.

@fritzmg fritzmg merged commit af80e6b into contao:main Oct 18, 2022
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

4 participants