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

always load all scripts in type/, command/ and handler/ #83

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rapus95
Copy link
Member

@rapus95 rapus95 commented Jun 7, 2021

No description provided.

@rapus95 rapus95 changed the title always load all scripts in command/ and handler/ always load all scripts in type/, command/ and handler/ Jun 8, 2021
Copy link
Member

@Azzaare Azzaare left a comment

Choose a reason for hiding this comment

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

I trust you ^^

Comment on lines +34 to +36
wd = walkdir(joinpath(@__DIR__, dir))
commands = first(wd)
close(wd)
Copy link
Member

Choose a reason for hiding this comment

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

Since it only works with single level, it's simpler to use readdir with join = true instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

but readdir is single level only aswell. Am I misunderstanding it?

Copy link
Member

Choose a reason for hiding this comment

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

Right, that’s why I suggested that. The code would be simpler since you don’t need to deal with the tuple from walkdir.

Copy link
Member Author

Choose a reason for hiding this comment

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

but on the other hand I only need the files (not the directories) and walkdir already sorts by type of element. readdir returns files and directories arbitrarily mixed.

@@ -24,29 +24,27 @@ import StructTypes

const COMMAND_PREFIX = get(ENV, "HOJBOT_COMMAND_PREFIX", ",")

function loadplugins()
Copy link
Member

Choose a reason for hiding this comment

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

Can we use underscore to separate words in function names? We try to follow the BlueStyle.
https://github.com/invenia/BlueStyle#function-naming

Copy link
Member Author

Choose a reason for hiding this comment

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

But Julia itself suggests to use underscores only if the name is otherwise unreadable: https://docs.julialang.org/en/v1/manual/style-guide/#Use-naming-conventions-consistent-with-Julia-base/
:/

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm aware of that. But coding style standard is made for a reason. The BlueStyle is quite widely used in the industry, and it has a good balance of consistency that tends to work well for larger projects.

RE: naming convention, the standard Julia recommendation is too loose in my humble opinion-- when does one decide when the smashed-together name is starting to become unreadable? It's subjective.

@tk3369
Copy link
Member

tk3369 commented Jun 21, 2021

Mmm… on a second thought, this would be broken if I have any source files from the same directories that must be loaded in a specific order, perhaps due to some dependency. There is no easy way to fix that…

@rapus95
Copy link
Member Author

rapus95 commented Jun 21, 2021

Mmm… on a second thought, this would be broken if I have any source files from the same directories that must be loaded in a specific order, perhaps due to some dependency. There is no easy way to fix that…

We could switch to making proper (sub)modules rather than bulk-including everything into the Main-Module

on the other hand, only types can get dependency problems. So for all stateless functions that don't rely on custom types, that should work. What was the reason that /type was split out of the other 2? I.e. why aren't the types included where they are needed?

@tk3369
Copy link
Member

tk3369 commented Jun 27, 2021

Separating type definitions allows you to use it in multiple files without worrying about the order of the included files. It's simpler to just place it at the beginning of includes than trying to figure out how to order the rest of the files.

@rapus95
Copy link
Member Author

rapus95 commented Jun 27, 2021

But why do multiple independent plugins need the same types? 👀 This already suggests some coupling which makes me think whether it should be designed differently anyway.

@tk3369
Copy link
Member

tk3369 commented Jun 28, 2021

Right, independent plugins don't generally need the same types -- except in the case that a common set of utilities are made available for the plugin's, which is quite conceivable. In any case, I think we should work on #11.

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.

3 participants