-
Notifications
You must be signed in to change notification settings - Fork 3
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
structure upload #57
structure upload #57
Conversation
@@ -15,3 +15,12 @@ class WorkspaceContainer(Container): | |||
A folder to contain WorkspaceFolders | |||
""" | |||
grok.implements(IWorkspaceContainer) | |||
|
|||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this import not be at the top of this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's slightly more readable this way, because the import is only used in that small block that is in itself a bit of an afterthought. That said, we probably want a hard dependency on ploneintranet.attachments ultimately, so we would turn it into an unconditional import and maybe also do the "implements" differently.
Actually, it's probably time to change that now. I'll look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A hard dependancy on ploneintranet.attachments? What functionality does IAttachmentStorage provide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... attachments. :-) From a user perspective, you can post a status update and attach a file to it. From a dev perspective, a file object is stored on the workspace (or workspace container) temporarily until the status update is finished, then the file is moved over and attached to the status update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That kind of functionality sounds like it should happen via a generic adapter in ploneintranet.attachments, rather than requiring the workspace to provide the interface manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, this could be a dexterity behaviour in ploneintranet.attachments. It could then be applied to a workspace using a policy egg that brings the two packages together. See ploneintranet/ploneintranet.attachments#4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Since attachments are not the focus of this pull request I would propose to merge now and refactor the attachment code afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamcheasley OK with you, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
Thanks! |
activated pat-upload