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

Add a config file and a force option to overwrite profiles #9

Merged
merged 6 commits into from Mar 5, 2021

Conversation

Gaareth
Copy link
Contributor

@Gaareth Gaareth commented Mar 2, 2021

This will add a config file using yaml, as I found this to be the easiest human readable format.

I have merged your folder_names and file_names variables into a single entries variable, as it removes some boilerplate code, in my opinion.

The config file gets copied to the right position on the executing of the install.sh script.

Note: This introduces a dependency: PyYaml, but I forgot to include a requirements.txt file, therefore I recommend to add one later.

Also I added a force option to overwrite already saved profiles, which I found quite useful

Furthermore I reformatted the file a bit, and turned the single triple quotes intro double triple quotes, as my IDE suggested this.

Note: This is my first pull request, so please tell me if it is okay.

@Prayag2
Copy link
Owner

Prayag2 commented Mar 2, 2021

The overwrite feature is a nice one to have but I think you forgot to add it here:
SS_2021-03-02_23-49-23

@Gaareth
Copy link
Contributor Author

Gaareth commented Mar 2, 2021

The overwrite feature is a nice one to have but I think you forgot to add it here:

SS_2021-03-02_23-49-23

No it actually works, the args.force variable gets passed to the save_profile function

@Prayag2 Prayag2 mentioned this pull request Mar 3, 2021
konsave Outdated
@@ -10,38 +10,73 @@
import os, shutil, argparse
from zipfile import ZipFile, ZIP_DEFLATED, is_zipfile

try:
import yaml
import bitches
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming you forgot to delete this line.

Copy link
Contributor Author

@Gaareth Gaareth Mar 3, 2021

Choose a reason for hiding this comment

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

Oh my fucking god. I'm really sorry, I did this in order to test if the catch works.

As this is my first pull request, how can I resolve this?
Edit: should be fixed by the latest commit.

konsave Outdated
'kdeglobals', 'kglobalshortcutsrc', 'klipperrc', 'krunnerrc', 'kscreenlockerrc',
'ksmserverrc', 'kwinrc', 'kwinrulesrc', 'plasma-org.kde.plasma.desktop-appletsrc',
'plasmarc', 'plasmashellrc', 'gtkrc', 'gtkrc-2.0', 'lattedockrc', 'breezerc', 'oxygenrc',
'lightlyrc', 'ksplashrc']
Copy link
Owner

Choose a reason for hiding this comment

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

I think we could download the config file for them instead of creating a new default_entries list. Here's what we can do:

import urllib.request
url = URL_OF_THE_CONF_FILE
destination = PATH_TO_DOWNLOAD_LOCATION
urllib.request.urlretrieve(url, destination)

urllib is a built in module so there's no problem in using it. This would improve the user experience.

konsave Outdated
Comment on lines 59 to 61
except urllib.error.HTTPError:
fallback_url = 'https://raw.githubusercontent.com/Gaareth/konsave/add-config-and-force-option/conf.yaml'
urllib.request.urlretrieve(fallback_url, CONFIG_FILE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a fallback URL in case yours does not work

@Prayag2
Copy link
Owner

Prayag2 commented Mar 4, 2021

I've updated a lot of things (bug fixes) so please update them in your repo too

Comment on lines +17 to +18
package_data={'config': ['conf.yaml']},
install_requires=['PyYaml'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the PyYaml requirement and added the config file to package_data

Comment on lines +41 to +45
if not os.path.exists(CONFIG_FILE):
print_msg(f"No config file found! Using default config ({default_config_path}).")
shutil.copy(default_config_path, CONFIG_FILE)
print_msg(f"Saved default config to: {CONFIG_FILE}")
return yaml.load(resource_stream('konsave', 'conf.yaml'), Loader=yaml.FullLoader)["entries"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the config file is not found the default config from the pip install folder will be copied to '~/.config/konsave'

Comment on lines 24 to 25
parser.add_argument('-f', '--force', required=False, action='store_true', help='Overwrite already saved profiles')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the force option

Comment on lines -16 to +15
os.mkdirs(PROFILES_DIR)
os.mkdir(PROFILES_DIR)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the bug I referenced in the commit message

@Prayag2
Copy link
Owner

Prayag2 commented Mar 5, 2021

Thanks! I'm merging your pull request now! But before that, could you update the README and change the version in setup.py to "1.0.4" too?

@Prayag2
Copy link
Owner

Prayag2 commented Mar 5, 2021

Please update the readme too. Just add a description for the "--force" option so that the users will know it exists.

@Gaareth
Copy link
Contributor Author

Gaareth commented Mar 5, 2021

Should work now :D

@Gaareth
Copy link
Contributor Author

Gaareth commented Mar 5, 2021

wait I think I found another bug D:

@Prayag2 Prayag2 merged commit 066f3a4 into Prayag2:master Mar 5, 2021
@Prayag2
Copy link
Owner

Prayag2 commented Mar 5, 2021

Thank you!

@Gaareth
Copy link
Contributor Author

Gaareth commented Mar 5, 2021

wait I think I found another bug D:

  File "/usr/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/gareth/tools/konsave/konsave/__main__.py", line 3, in <module>
    from konsave.funcs import *
  File "/home/gareth/tools/konsave/konsave/funcs.py", line 5, in <module>
    from konsave.vars import *
  File "/home/gareth/tools/konsave/konsave/vars.py", line 15, in <module>
    os.mkdir(PROFILES_DIR)
FileNotFoundError: [Errno 2] No such file or directory: '/home/gareth/.config/konsave/profiles'

This fixes it:

if not os.path.exists(KONSAVE_DIR):
    os.mkdir(KONSAVE_DIR)

@Prayag2
Copy link
Owner

Prayag2 commented Mar 5, 2021

I'm getting this now:
SS_2021-03-05_23-50-45
Please make a pull request and change the location of the config file to "~/.config/konsave/".

@Gaareth
Copy link
Contributor Author

Gaareth commented Mar 5, 2021

I'm getting this now:
SS_2021-03-05_23-50-45
Please make a pull request and change the location of the config file to "~/.config/konsave/".

eh, have you already reinstalled the package? python -m pip install -e <the konsave dir>

@Prayag2
Copy link
Owner

Prayag2 commented Mar 5, 2021

I just ran python -m pip install konsave==1.0.5

@Gaareth
Copy link
Contributor Author

Gaareth commented Mar 5, 2021

I am unsure with the process, how this directory get synced with pip,
but could you try, installing it manually: python -m pip install -e <the konsave dir>

@Gaareth
Copy link
Contributor Author

Gaareth commented Mar 5, 2021

The pull request should hopefully fix this
#16

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.

None yet

3 participants