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

Tk framework adobe Code Review #1

Merged
merged 34 commits into from
Mar 14, 2019
Merged

Tk framework adobe Code Review #1

merged 34 commits into from
Mar 14, 2019

Conversation

martinminsel
Copy link
Contributor

First pass review for the general structure
This is not ment to be merged yet

@martinminsel martinminsel self-assigned this Jan 22, 2019
@martinminsel
Copy link
Contributor Author

I updated this pull request with the latest code. Ready for a detailed code review.
Please Note:

In order to make it work you need to get the tk-framework-adobe branches of the following projects:
tk-multi-launchapp
tk-config-default2/tk-config-basic
tk-aftereffectscc
tk-photoshopcc
tk-framework-adobe

Be aware, that you have to configure the app-locations in the configs accordingly.

@martinminsel martinminsel changed the title [WIP] Tk framework adobe First Pass Review Tk framework adobe Code Review Feb 8, 2019
@thebeeland thebeeland requested review from thebeeland and removed request for robblau and manneohrstrom February 14, 2019 23:31
Copy link
Contributor

@thebeeland thebeeland left a comment

Choose a reason for hiding this comment

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

Whew...that's a big framework!

In general I think it's looking really good. A lot of the comments I have are pretty minor in scope, so hopefully a second pass at it won't take too much effort. I would like to take another look at it once you've gone through all of my comments.

Thanks so much for all of the hard work!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
framework.py Outdated Show resolved Hide resolved
framework.py Outdated Show resolved Hide resolved
info.yml Outdated Show resolved Hide resolved
python/adobe_bridge.py Outdated Show resolved Hide resolved
python/shotgunutils.py Outdated Show resolved Hide resolved
@martinminsel
Copy link
Contributor Author

Hi @thebeeland ,
I did the changes for this. Thanks for your detailed review.
Let me know your thoughts.
Cheers.

Copy link
Contributor

@thebeeland thebeeland left a comment

Choose a reason for hiding this comment

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

The changes you made look good. The only remaining comments I have are around the README, so if you can tackle those few things I think we're in pretty good shape here.

I'll take one last look at the README once you've made a pass at that.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
dev/env.mk Show resolved Hide resolved
@thebeeland
Copy link
Contributor

One more note now that I've caught up with Manne and Rob. It sounds like Sphinx docs are also on the table as something that's needed to be done. We have examples of how to piece that together in our other frameworks (qtwidgets is a great example). I'm happy to review that work, as well, once it's ready. Just let us know!

@manneohrstrom
Copy link
Contributor

manneohrstrom commented Feb 21, 2019

Just a quick note as i am looking at this - possible that @thebeeland has already mentioned it - but we should rename the folder python/tk_adobe_basic to python/tk_framework_adobe so that we follow our naming conventions.

@manneohrstrom
Copy link
Contributor

manneohrstrom commented Mar 8, 2019

This looks good to me (apart from the missing sphinx docs). I say we merge this with master and do the sphinx docs (#2 ) as a separate PR and branch. I will ping the @thebeeland to remind him to take a look and give his ✅

Copy link
Contributor

@thebeeland thebeeland left a comment

Choose a reason for hiding this comment

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

I like it. Solid effort all around! 👍

@martinminsel martinminsel merged commit 7bac887 into master Mar 14, 2019
@manneohrstrom manneohrstrom deleted the tk-framework-adobe branch March 14, 2019 13:39
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