Skip to content

Conversation

Oreeeee
Copy link

@Oreeeee Oreeeee commented May 13, 2022

Storing credentials in .py file is very unsafe since you can accidentally commit your password and loading from external file will make it easier to modify and update the script.

@Oreeeee Oreeeee mentioned this pull request May 14, 2022
@marco44
Copy link

marco44 commented Jul 9, 2023

Another reason for having the password in a config file is packages of this file, like https://aur.archlinux.org/packages/opensubtitlesdownload

You can't really edit a file that's part of a package. Well, it defeats the purpose of having a package that's going to be managed by the system

@Oreeeee
Copy link
Author

Oreeeee commented Jul 9, 2023

Then maybe adding the file to a different location like ~/.config/OpenSubtitlesDownload/config.json for Unix and %UserProfile%\Documents\OpenSubtitlesDownload\config.json would be a better idea?

@marco44
Copy link

marco44 commented Jul 9, 2023

Oh, currently it's in the current working directory ? Yeah it would make much more sense I think…

@Oreeeee
Copy link
Author

Oreeeee commented Jul 9, 2023

I misunderstood the comment you made. I thought you were saying that the issue was that the config file was in current directory, and not that it is currently stored in the code. I made this PR over a year ago and I didn't remember the context. Anyways, I'm gonna move the config to the locations I mentioned.

@Oreeeee
Copy link
Author

Oreeeee commented Jul 9, 2023

It should be now changed.

@marco44
Copy link

marco44 commented Jul 9, 2023

I'm not a dev on this project. I was just stating that as another user, I would like this feature too, it's not practical passing the user/pass each time :)

@Kyshman
Copy link

Kyshman commented Feb 4, 2024

Would this force someone to use the json file for credentials or would be optional for those who want to use it?

@Oreeeee
Copy link
Author

Oreeeee commented Feb 4, 2024

@Kyshman This forces the user to use the JSON.

@Oreeeee
Copy link
Author

Oreeeee commented Feb 4, 2024

Actually, looks like v6.2 includes a much better way of handling downloads for free users, shipping a hard-coded API key, and logging in being optional for paid users. But I won't close the PR, as the settings are still hard-coded.

@emericg
Copy link
Owner

emericg commented Mar 27, 2024

Hi, sorry I never answered this pull request, I didn't put much work in this software for the past couple of years...

@Kyshman This forces the user to use the JSON.

The problem is indeed that this change fundamentally how the software works. If the settings from the JSON file were just superseding the settings from the Python file, it would have been acceptable.
The settings file path selection is also not very solid.

I have no problem letting this PR open however, if people want to use it or improve upon it.

@thiswillbeyourgithub
Copy link

thiswillbeyourgithub commented Oct 19, 2025

In my script usually what I do is use environment variables as a fallback for args. You can use the same principle to load a json without forcing the user to know json

Example here: https://github.com/thiswillbeyourgithub/freshrss_python_api/blob/6c12feea52f9a2adac61e17c683322904dc52837/freshrss_api/freshrss_api.py#L58

@thiswillbeyourgithub
Copy link

Also I recommend using platformdirs to easily find the appropriate config path

# Load config from JSON file
try:
cfg = json.load(open(cfg_file))
except FileNotFoundError:

Choose a reason for hiding this comment

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

This crashes if the json is malformed no?

@emericg
Copy link
Owner

emericg commented Oct 21, 2025

In my script usually what I do is use environment variables as a fallback for args. You can use the same principle to load a json without forcing the user to know json

That is a very interesting and non-intrusive solution, I may add that!
Not for the json, but for the username / password like in your example.

Also I recommend using platformdirs to easily find the appropriate config path

No third party modules in this script, just stock python.

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.

5 participants