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

Fix most luacheck warnings and make the bot more modular #224

Merged
merged 10 commits into from
Sep 15, 2017
Merged

Fix most luacheck warnings and make the bot more modular #224

merged 10 commits into from
Sep 15, 2017

Conversation

yangm97
Copy link
Member

@yangm97 yangm97 commented Jul 30, 2017

From 1335 warnings to 11, I think that’s a pretty good start 😁

Overview:

  • Change dkjson back to lua-cjson
  • Renamed lua/bot.lua to lua/main.lua
  • Moved polling logic from lua/main.lua to polling.lua
  • The only remaining global variable is bot (already added to .luacheckrc globals), mostly to keep the admin commands $init and $stats working. Could easily avoid that by using a module instead.

Things to look after:

  • I did my best while fixing “line is too long” warnings but some strings might need to be readjusted. Using a short id (instead of the english string) in source code would be a better IMHO
  • $init doesn’t seem to be reloading the modules, possibly due to the fact none of them are global anymore. I think it may be better to replace it with something like $stop and rely on the supervisor (docker, systemd, launch.sh…) to relaunch
  • This builds on top of the previous docker PR. Simply merging this should be enough to close both PRs

@yangm97
Copy link
Member Author

yangm97 commented Jul 31, 2017

Hold on, trow away variable _ is conflicting with localisation variable _. Don’t merge until the name of the latter is changed to i18n or something.

@yangm97
Copy link
Member Author

yangm97 commented Aug 2, 2017

Fixed. Should be ready to go now.

@Synk0
Copy link
Contributor

Synk0 commented Aug 15, 2017

What is the status of this?

Signed-off-by: Yan Minari <yangm97@gmail.com>
Signed-off-by: Yan Minari <yangm97@gmail.com>
Signed-off-by: Yan Minari <yangm97@gmail.com>
Signed-off-by: Yan Minari <yangm97@gmail.com>
Signed-off-by: Yan Minari <yangm97@gmail.com>
Signed-off-by: Yan Minari <yangm97@gmail.com>
- The only remaining global variable is bot, mostly to keep $stats working. Could avoid using a global by using a module instead
- Move polling logic to polling.lua
- Some strings may need to be readjusted

Signed-off-by: Yan Minari <yangm97@gmail.com>
Signed-off-by: Yan Minari <yangm97@gmail.com>
Translation now uses i18n instead of _

Signed-off-by: Yan Minari <yangm97@gmail.com>
There’s only 7 warnings remaining now

Signed-off-by: Yan Minari <yangm97@gmail.com>
@NotAFile
Copy link
Collaborator

until the name of the latter is changed to i18n or something

This breaks the translation unfortunately. Gettext relies on scanning the file for _(...) to find translatable strings. You can change this prefix, but it has to be consistent for the collection to work.

@yangm97
Copy link
Member Author

yangm97 commented Sep 11, 2017

Which file need to be changed?

@NotAFile
Copy link
Collaborator

Well, everything. The "localisation variable" needs to be the same everywhere. We could rename it to something like _T(...) or whatever, but it needs to be the same everywhere

@yangm97
Copy link
Member Author

yangm97 commented Sep 11, 2017

@NotAFile well, I’ve picked up i18n() as the localisation variable and used it everywhere (on the lua side of things)

@yangm97
Copy link
Member Author

yangm97 commented Sep 11, 2017

I’m not sure what’s needed to do on the gettext script though, in fact I even forgot it existed.

@RememberTheAir RememberTheAir changed the base branch from beta to staging September 14, 2017 19:56
@RememberTheAir RememberTheAir changed the base branch from staging to master September 15, 2017 22:41
@RememberTheAir RememberTheAir merged commit 25092cc into group-butler:master Sep 15, 2017
@yangm97 yangm97 deleted the fix-warnings-and-make-modular branch October 16, 2017 16:59
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