-
Notifications
You must be signed in to change notification settings - Fork 10
issue-#2 sphinxdocs #7
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
base: master
Are you sure you want to change the base?
Conversation
…are/tk-framework-adobe.git into 2-sphinxdocs
|
Note - this addresses issue #2 |
|
|
||
|
|
||
|
|
||
| class MessageEmitter(QtCore.QObject): |
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.
too many blank lines (3)
| from .plugin_init import toolkit_plugin_bootstrap | ||
|
|
||
| from .adobe_bridge import Communicator | ||
| from .errors import RPCTimeoutError |
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.
'.errors.RPCTimeoutError' imported but unused
| from .classic_init import toolkit_classic_bootstrap | ||
| from .plugin_init import toolkit_plugin_bootstrap | ||
|
|
||
| from .adobe_bridge import Communicator |
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.
'.adobe_bridge.Communicator' imported but unused
| active_document_changed = QtCore.Signal(str) | ||
| from .rpc import Communicator | ||
| from .utils import timeout, MessageEmitter | ||
| from .errors import RPCTimeoutError |
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.
'.errors.RPCTimeoutError' imported but unused
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.
Hey Martin,
I did a bunch of tweaks to the doc to improve the DX a bit. You can find the latest build here:
This is the first time i am diving into the framework properly, and there are some things i would like us to change in the structure. I will chat to @thebeeland to make sure i understand the context properly, but so far i have ticketed up some stuff we need to follow up on:
- #10 - The public API interface is not defined properly. This is important to do so that we can make changes and refactor in the future.
- #9 - There are environment variables that need cleaning up.
- #8 - There is photoshop specific logic that needs to be moved out of the FW.
- #12 - Unit tests for framework
We can leave that for later. For now, so we can get the docs into an ok place, it would be great if you could:
- Check my changes - see that they make sense to you and that the content seems right. Feel free to make any changes you see relevant!
- Are there any other classes or methods that form part of the public API for the fw? In that case we need to add them in.
- Go over all methods that you find in the sphinx docs, make sure that they have all explicit parameters defined (no
**kwargsor*argsunless we explicitly have designed a method for this purpose) - for example i see the constructor missing this. I also noticedget_or_createis missing this. We want to be explicit, not implicit, whenever we can in our interfaces. - The constructor docs should explain how we want users to contruct the bridge (e.g. use the factory method provided).
- Make sure that all methods document their parameters, return value, if they raise anything and have a meaningful description of that they do. I noticed some descriptions where being cut off half way - cut and paste error perhaps?
- Please do a pass to ensure that all methods that are public are actually stuff we want to expose in our public API - for each one, what's the need and story behind them for a user who wants to implement a new engine? If there isn't a need, we keep them internal by prefixing with an underscore. For example, is
response_logging_silencedpublic or private? If public, please add an example to explain how a developer of an engine is supposed to use this as part of their integrations. Similar withrpc_is_equal- seems internal to me, but i might be wrong? - why is
plugin_init.pyandclassic_init.pyinsidetk_framework_adobewhen they are being called fromtk_framework_adobe_utils? Seems they need to be moved. - I am still seeing missing docstrings for some methods (e.g
get_adobe_cep_dir) - please do a pass to ensure all methods have docstrings and parameters! - Looks like the icon for the
cepextension is wrong - still contains a PS icon!
Any questions, just ping! Thanks!
|
One more note: The docs are also missing information on the general workflow how to bootstrap an engine which is using the CEP - we have a ticket to refactor the core environment logic - i will make a note to make sure we add docs here in the framework at the same time. |
This is the first version. What do you think?