|
I've scanned the source and I think you have caught all the instances that need changing.
Are we sure we want to use CamelCase in the option names? I think this may be confusing if there is not a 1:1 correspondence between the PythonOption key and actual module namespace. So should it be mod_python.FileSession.some_option or mod_python.session.FileSession.some_option? I'd rather not use CamelCase if we don't have a 1:1 mapping between the PythonOption key and the fully qualified class name. So either use mod_python.file_session.fast_cleanup or mod_python.session.FileSession.fast_cleanup I'd avoid using mod_python.BaseSession, and just use mod_python.session for global session options. Using BaseSession just exposes an implementation detail and does not make the intent any clearer. Plus Again, notice the potential confusion with the camel case. session.Session is a factory function, so should mod_python.Session.some_option only control the behaviour of that function, or is it intended as global option for all session classes? What if a user creates a session instance directly, bypassing session.Session() - should mod_python.Session.some_option still be used? I think for your proposed changes for the FileSession options are logical and should be adopted. I wonder though if we should refactor the constructor to use the new names as well? It would certainly be easier to do this now as FileSession may not be widely adopted yet. Consistency is a good thing after all. There is no conflict using session_directory for both DbmSession and FileSession. DbmSession creates a dbm file (/session/directory/mp_sess.dbm) in the directory, while FileSession creates its file hiearchy its own subdirectory (/session/directory/mp_sess/). Of course people are free to use mod_python.DbmSession.database_filename /session/directory/mp_sess to mess things up, but there is not much we can do about that. My original thinking was that the session directory would be global to any session class that needs it. Should that directory flip back to the default (/tmp) just because I decide to use a different session class? Of course we could have it both ways: if 'mod_python.session.FileSession.database_directory' in options use it else use mod_python.session.database_directory Okay, lets not use camel case and where a sub namespace is required, it should refer to the concept of what is being implemented as opposed to a specific class name. Thus two general forms would be:
mod_python.global_option_name mod_python.feature_name.sub_option_name Thus: psp.py # class PSP PSPDbmCache ==> mod_python.psp.cache_database_filename Session.py # class BaseSession session_cookie_name ==> mod_python.session.cookie_name ApplicationPath ==> mod_python.session.application_path # class DbmSession session_dbm ==> mod_python.dbm_session.database_filename session_directory ==> mod_python.dbm_session.database_directory # class FileSession session_fast_cleanup ==> mod_python.file_session.enable_fast_cleanup session_verify_cleanup ==> mod_python.file_session.verify_session_timeout session_grace_period ==> mod_python.file_session.cleanup_grace_period session_cleanup_time_limit ==> mod_python.file_session.cleanup_time_limit session_directory ==> mod_python.file_session.database_directory # class Session session ==> mod_python.session.session_type Documentation would state that any option of form mod_python.* is reserved for mod_python use. Documentation should also encourage users when creating add-ons to use their own namespace qualifier and not pollute the global namespace. In respect of backward compatibility for old option names, although it would be logical to support old names, I would suggest we add an option: mod_python.allow_old_option_names For now this would default to "1" indicating that old names would still be supported. It could though be used to turn off acceptance of old names. Come the next major version, I would suggest the default for this option change to "0" indicating that by default old names would not be accepted. At that time if you really need to use the old names, then you can enable the support. Lets target this to be done for 3.3. We just need some agreement that proposed names are okay, plus a consensus on how we go about deprecating old names. Do we have an option now which if enabled prohibits use of old names and outputs some sort of warning if they are, or do we do something else?
Have made code changes (not documentation), but not yet committed.
Having done this, I found myself asking whether there should also be a: mod_python.session.database_directory This would be a fallback for when the 'session_database' option against a specific type of session class hasn't been set. Ie., search order would be: mod_python.dbm_session.database_directory mod_python.session.database_directory session_directory # for backward compatibility only and: mod_python.file_session.database_directory mod_python.session.database_directory session_directory # for backward compatibility only This would allow session directory to be set once regardless of what type of session class was used. At least, for those which store stuff in the file system, as MemorySession wouldn't use it. Comments??? +1
Your suggestion is logical. Explicit is better than implict. We need to make sure we address this in the unittests as well. Should we have tests for the additional options?
For now I have just made the tests use the new option names. There is obviously a risk that I stuffed up the backward compatibility for old options, but hopefully not. Documentation updated where appropriate.
|
||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
Use namespace for mod_python PythonOption settings.
psp.py
# class PSP
PSPDbmCache ==> mod_python.PSP.cache_database_filename
Session.py
# class BaseSession
session_cookie_name ==> mod_python.BaseSession.cookie_name
ApplicationPath ==> mod_python.BaseSession.application_path
# class DbmSession
session_dbm ==> mod_python.DbmSession.database_filename
session_directory ==> mod_python.DbmSession.database_directory
# class FileSession
session_fast_cleanup ==> mod_python.FileSession.enable_fast_cleanup
session_verify_cleanup ==> mod_python.FileSession.verify_session_timeout
session_grace_period ==> mod_python.FileSession.cleanup_grace_period
session_cleanup_time_limit ==> mod_python.FileSession.cleanup_time_limit
session_directory ==> mod_python.FileSession.database_directory
# class Session
session ==> mod_python.Session.session_type
Anyone like to confirm that this is all and validate that names reasonable or suggest better names. My choices of names for FileSession, although they now have a name which is clearer to me, mean they diverge from what class initialiser variable names were. If all session types derive from BaseSession, should the existance of the BaseSession class be hidden and mod_python.Session.* be used instead of cases where list mod_python.BaseSession.*?
Note that an interesting issue deriving from making this list is that both DbmSession and FileSession use the "session_directory" option at the moment. Could a conflict arise from doing this if they are both writing to the same directory?
Feedback?