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

Move packages/python/plotly to top level #5002

Merged
merged 10 commits into from
Feb 4, 2025
Merged

Conversation

emilykl
Copy link
Contributor

@emilykl emilykl commented Jan 30, 2025

  • Moves the contents of packages/python/plotly to the top level of the repository
  • Updates paths across the repository to correspond to the new location

The full diff is pretty unwieldy, Github does not perform well with large diffs.

Link to the diff which excludes the commit where files were moved (more manageable): https://github.com/plotly/plotly.py/pull/5002/files/32adcc3de1cf3f6d15b35a4ed85e981bc9a8b24b..54d076b7c734d99e8efe9a96491cbe6b09adbc3c

@emilykl emilykl force-pushed the simplify-dir-structure branch from a2c3bf5 to f642b58 Compare January 31, 2025 23:17
@emilykl emilykl changed the title move packages/python/plotly to top level Move packages/python/plotly to top level Feb 1, 2025
@gvwilson gvwilson added P1 needed for current cycle infrastructure build process etc. labels Feb 3, 2025
@emilykl emilykl marked this pull request as ready for review February 3, 2025 21:33
@marthacryan
Copy link
Collaborator

So sad that github diff doesn't work with this PR 😭 I'll just leave comments on what I find here because I basically can't leave comments in the diff.

  • README.md could include a link to the plotly-geo repo since that has its own section there. Just in case people want to make issues or even just to track down where it is.
  • Looks like the .gitignore needs to be updated - specifically:
packages/javascript/jupyterlab-plotly/lib/
plotly/jupyterlab_plotly/labextension/
plotly/jupyterlab_plotly/nbextension/index.js*
  • I feel like we should remove the whole section on tox in the CONTRIBUTING.md but that's for a different PR

Overall, I think this is ready to merge even if there will be some finishing touches afterward.

@gvwilson
Copy link
Contributor

gvwilson commented Feb 4, 2025

@emilykl would you like to push the big green button or shall I?

@emilykl
Copy link
Contributor Author

emilykl commented Feb 4, 2025

@gvwilson I'll make the changes Martha suggested and then merge!

@LiamConnors
Copy link
Member

The make file for the API reference docs needs an update for the new structure:
https://github.com/plotly/plotly.py/blob/simplify-dir-structure/doc/apidoc/Makefile
It still references packages/python...

@emilykl
Copy link
Contributor Author

emilykl commented Feb 4, 2025

Thanks @LiamConnors , good catch.

I've updated the paths -- how would you recommend I verify that the Makefile is correct now?

@LiamConnors
Copy link
Member

Thanks @LiamConnors , good catch.

I've updated the paths -- how would you recommend I verify that the Makefile is correct now?

If you install the requirements.txt in doc and then go to doc/apidoc, running make html should generate the output.
I've tested it with these changes and the API docs build correctly.

@emilykl emilykl merged commit 5813a8a into master Feb 4, 2025
5 checks passed
@emilykl emilykl deleted the simplify-dir-structure branch February 4, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure build process etc. P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants