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

Modular module system #380

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Modular module system #380

wants to merge 9 commits into from

Conversation

dorbian
Copy link

@dorbian dorbian commented Nov 18, 2014

Created a system that keeps the module config with the modules, loads the modules that have an ini file with them.
ini file can be blank as well when the module doesn't require any configuration.

Dorbian and others added 6 commits November 4, 2014 00:16
modular system for importing modules
Modular import working
@gugahoi
Copy link
Collaborator

gugahoi commented Nov 18, 2014

@mrkipling @N3MIS15 You should check this out. Small change that makes a big difference in modularity for new modules. Would like a second opinion before merging 😄

@Hellowlol
Copy link

Might be wise to have a sample and add the inis to git ignore?

@@ -33,3 +33,11 @@ ehthumbs.db
Thumbs.db
.directory
*~
.idea/.name
Copy link
Owner

Choose a reason for hiding this comment

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

Are these additions to .gitignore needed? I can't see what they're for.

@mrkipling
Copy link
Owner

Might be wise to have a sample and add the inis to git ignore?

Wouldn't that mean that the modules don't load out of the box, though? The way I see it this isn't really so that users can edit the ini files.

@mrkipling
Copy link
Owner

I haven't tested this yet, but generally it gets my +1!

@N3MIS15
Copy link
Collaborator

N3MIS15 commented Nov 19, 2014

I would be happy to review, but I won't have an opportunity until the weekend. @gugahoi ping me on the weekend so I remember.

@Hellowlol
Copy link

I was under the impression that the users would edit the inis, if that isnt the case there would no reason to add it to git ingore.

@mrkipling
Copy link
Owner

It's entirely possible that I'm just being dense, it's early and I'm still on coffee 1 :)

If users change settings by editing the ini files, does this mean that you can no longer change settings from within the interface? Because that wouldn't be so great.

Perhaps we could get some clarification on what exactly is proposed here? Modular modules are good; extra complexity for the user is bad.

@gugahoi
Copy link
Collaborator

gugahoi commented Nov 19, 2014

I will clarify some stuff for you guys:

The ini files are simply the default settings for the modules, user settings still reside in the db. All this does is split the long ass array of default settings we used to have into their dedicated files. Makes it easier to manage individual modules. That stuff added to gitignore it not necessary, his own IDE added that in.

It's great to hear your opinions tho 👍

@mrkipling
Copy link
Owner

The ini files are simply the default settings for the modules, user settings still reside in the db. All this does is split the long ass array of default settings we used to have into their dedicated files.

Fantastic, this is my original assumption. I'm all for that, sounds like a great idea.

@@ -1040,6 +340,7 @@ def extra_settings_dialog(dialog_type, updated=False):

if dialog_type == 'search_settings':
settings = copy.copy(SEARCH_SETTINGS)
print settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

this shouldn't be here..

@N3MIS15
Copy link
Collaborator

N3MIS15 commented Nov 19, 2014

without testing, In theory it looks sound.
Again, please ping me on the weekend.

@dorbian
Copy link
Author

dorbian commented Nov 19, 2014

Sorry bout that, thought i removed all of them. Will get that out asap
On Nov 19, 2014 12:35 PM, "David Gray" notifications@github.com wrote:

without testing, In theory it looks sound.
Again, please ping me on the weekend.


Reply to this email directly or view it on GitHub
#380 (comment).

Checked .ini files for other python code execution, which is possible,
however the .ini files are there for default config, which shouldn't
contain any other information, can rename them all to something
different if needed.
@dorbian
Copy link
Author

dorbian commented Nov 19, 2014

Removed the print statements ( all of them that i put in )
The ini files can contain other python code that can be executed ( ran a test on that ) but as this should contain nothing other then default config, there is no reason to add other python code in.

the .idea folder contains a lot of information from my IDE, didn't want that synced in so i added it to my .gitignore, the gitignore additions can be ignored.

I have been looking into a better way to make this more modular, however there are some files that look for the files in the location they are right now, to get this done i have to check what files they are and modify them as well.

The only downside i noticed is the sorting of the list in the GUI, currently this is not in the same sorting as it used to be as it is now generated.

@mrkipling
Copy link
Owner

the .idea folder contains a lot of information from my IDE, didn't want that synced in so i added it to my .gitignore, the gitignore additions can be ignored.

Could you adjust the PR to remove these additions please?

FYI you can add personal files that you want excluded to /path/to/maraschino/.git/info/exclude.

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

5 participants