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

Remove deprecated classic theme files #58

Closed
wants to merge 0 commits into from

Conversation

richardissuperman
Copy link
Collaborator

  1. Remove cd build/ as python files are with the same dir
  2. use absolute path so the script can work wherever dev run sh command

@richardissuperman
Copy link
Collaborator Author

TODO: update the README.md's animation gif

@richardissuperman richardissuperman changed the title use absolute path for running theme script Remove deprecated classic theme files Mar 8, 2025
launchUI.bash Outdated
echo "light theme selected"
#start light theme
python ../mode_dark_light.py --theme light
python $SCRIPT_DIR/mode_dark_light.py --theme light
Copy link
Owner

Choose a reason for hiding this comment

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

We may need ../ as the mode_dark_light.py invokes many sh commands with fixed relative path, e.g., mkdir os.system("mkdir test") or sh ../tools/sth, which assumes every generated file is kept inside a build folder. This is not good, but it is the current status, plz feel free to clean them up.

@yuqisun Plz confirm my statement. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you can find many os.chdir with relative path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yuqisun @tancheng interesting, so in my docker container i dont even see this build/ directory. Is there any missing steps i didnt perform to make the build/ folder and artifacts?
Screenshot 2025-03-09 at 6 36 18 PM

I was confused why there was a cd build/ in the scripts? We can discuss on monday meeting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hi @richardissuperman , looks the build folder is missed in image. There should be error when start from python launch.py if there's build folder right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yuqisun yes thats why I always just change this locally. I was thinking if you guys run any additional commands to generate build folder.

Copy link
Owner

Choose a reason for hiding this comment

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

@yuqisun yes thats why I always just change this locally. I was thinking if you guys run any additional commands to generate build folder.

We can add the command of creating build if it doesn't exit?

@tancheng tancheng requested a review from yuqisun March 9, 2025 00:19
@tancheng tancheng added documentation Improvements or additions to documentation cleanup Code cleanup labels Mar 9, 2025
@tancheng
Copy link
Owner

tancheng commented Mar 9, 2025

BTW, can you help create a doc directory and move both theme and debug into it?

@richardissuperman
Copy link
Collaborator Author

BTW, can you help create a doc directory and move both theme and debug into it?

@tancheng
sure happy to do that! few questions:

  1. Shall we also move my mac debugging tips into doc dir as well?
  2. Pictures under theme are also referenced as part of the source code inside launch.python, do we still want to move it under doc? I thought theme/ is part of the assets used by code not document (No.2 doesnt really matter that much though, just asking)

@tancheng
Copy link
Owner

Hi @richardissuperman, let's move all of them into the /doc. And change the path in the code accordingly. The docker dbugging anyway can be directed to the location from the root README. Thanks~!

@richardissuperman
Copy link
Collaborator Author

I accidentally close this by force push my master branch, but this is fine, lets address #60 first and later after discuss comment# i can reopen this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code cleanup documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants