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

Added required libraries and altered camera_selector python file #728

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

anirudh-hegde
Copy link

In camera_selector.py:
avat

In requirements.txt:
req

@JohanAR
Copy link
Collaborator

JohanAR commented Jun 1, 2024

Hi, unfortunately avatarify-python hasn't been actively maintained for quite some time. I can still accept minor easily reviewable changes, like this one, but I think the project as a whole is in pretty bad shape currently.

Regarding this change, I don't want to force everybody to create an avatarify-python folder in their home dir for the config. But if you want to improve this, perhaps you could make a list of different locations to search for config.yaml? I also think ~/.config/avatarify-python/ is preferred to putting folders directly under ~ on Linux, but I don't know if it's standard on all distributions, perhaps there's a method to get the path to the system's config directory? I also don't know if os.path.expanduser('~') works on Windows, but the code needs to work there as well, preferably using a folder which is consistent with other apps on that platform.

And finally, please make sure to remove the commented out code from the pull request, since there isn't any point on putting that on the main branch.

@anirudh-hegde
Copy link
Author

Hi, unfortunately avatarify-python hasn't been actively maintained for quite some time. I can still accept minor easily reviewable changes, like this one, but I think the project as a whole is in pretty bad shape currently.

Regarding this change, I don't want to force everybody to create an avatarify-python folder in their home dir for the config. But if you want to improve this, perhaps you could make a list of different locations to search for config.yaml? I also think ~/.config/avatarify-python/ is preferred to putting folders directly under ~ on Linux, but I don't know if it's standard on all distributions, perhaps there's a method to get the path to the system's config directory? I also don't know if os.path.expanduser('~') works on Windows, but the code needs to work there as well, preferably using a folder which is consistent with other apps on that platform.

And finally, please make sure to remove the commented out code from the pull request, since there isn't any point on putting that on the main branch.

Screenshot 2024-06-01 at 16-16-21 os path — Common pathname manipulations

https://docs.python.org/3/library/os.path.html

@JohanAR
Copy link
Collaborator

JohanAR commented Jun 1, 2024

Great that you checked for cross-platform compatibility! Unfortunately I don't think the current code works as intended. For example I have avatarify-python checked out to the path /mnt/storage/Programs/avatarify, so the expression parent_dir = os.path.basename(os.path.dirname(os.getcwd())) sets parent_dir to "Programs". Then the following expression yml_file_path = os.path.join(os.path.expanduser('~'), parent_dir, 'config.yaml') sets yml_file_path to "/home/johan/Programs/config.yaml" on my system.

To clarify, I think the best solution would be to create a list of possible locations for this file, for example ['config.yaml', '/.config/avatarify-python/config.yaml', '/avatarify-python/config.yaml'], and then loop through it and try to load the file from each location. If it successfully loads the file it breaks out of the loop, if it fails then it continues with the next. And using expanduser() and others to build the actual path, I just wrote it as a plain string to make it easier to read.

@anirudh-hegde
Copy link
Author

Great that you checked for cross-platform compatibility! Unfortunately I don't think the current code works as intended. For example I have avatarify-python checked out to the path /mnt/storage/Programs/avatarify, so the expression parent_dir = os.path.basename(os.path.dirname(os.getcwd())) sets parent_dir to "Programs". Then the following expression yml_file_path = os.path.join(os.path.expanduser('~'), parent_dir, 'config.yaml') sets yml_file_path to "/home/johan/Programs/config.yaml" on my system.

To clarify, I think the best solution would be to create a list of possible locations for this file, for example ['config.yaml', '/.config/avatarify-python/config.yaml', '/avatarify-python/config.yaml'], and then loop through it and try to load the file from each location. If it successfully loads the file it breaks out of the loop, if it fails then it continues with the next. And using expanduser() and others to build the actual path, I just wrote it as a plain string to make it easier to read.

Definitely will give a try

@anirudh-hegde
Copy link
Author

Hi @JohanAR, can you please review my code and suggest any changes needed.

parent_dir = os.path.basename(os.path.dirname(os.getcwd()))
file_location = ['Desktop', 'Downloads', 'Documents', parent_dir, '', 'Programs']
for loc in file_location:
yml_file_path = os.path.join(os.path.expanduser('~'), loc, 'config.yaml')
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're still effectively only looking in the user's home folder, it needs to work regardless of where they have downloaded it. Joining parent_dir with the home folder also doesn't make much sense, but maybe this is a bug and you intended to do something else? I'm not quite sure if it's logical that someone would have "~/Downloads/config.yaml" while the actual application is somewhere else.

yml_file_path = os.path.join(os.path.expanduser('~'), loc, 'config.yaml')
if os.path.exists(yml_file_path):
with open(yml_file_path, "r") as f:
config = yaml.load(f, Loader=yaml.FullLoader)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be a good idea to check that a config was loaded, and print a helpful error message if it wasn't found in any of the locations. Since the new code doesn't fail when it can't find the config file, it would probably lead to a more cryptic error later.

@JohanAR
Copy link
Collaborator

JohanAR commented Jun 14, 2024

Sorry for the delay, have been busy.

As a general tip, try testing your code locally more and check that it would look in the correct locations, regardless of where the user has downloaded avatarify-python. During development you can add some debug prints for each place it searches for the config file

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.

2 participants