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

update feature #535

Merged
merged 4 commits into from
Jul 15, 2023
Merged

update feature #535

merged 4 commits into from
Jul 15, 2023

Conversation

cosmos1721
Copy link
Contributor

update feature in continuation with closed PR #526 and issue #341

snap/snapcraft.yaml Outdated Show resolved Hide resolved
@AdnanHodzic
Copy link
Owner

Looks great, this is what I wanted #526 to look like 🙂

Before I merge this there's one minor change that needs to be done. Added as a comment for snap/snapcraft.yaml

snap/snapcraft.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@AdnanHodzic AdnanHodzic merged commit 3dffa23 into AdnanHodzic:master Jul 15, 2023
@AdnanHodzic
Copy link
Owner

LGTM! Thank you for your contribution, and your perseverance is admired in this process. 🙂

You will be credited for your work as part of future release and your future contributors are more then welcome!

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Jul 15, 2023

@cosmos1721 changes are already merged and I didn't want to mention it as part of initial PR. But I already have a few suggestions how to improve this functionality:

It's a good idea to update README section will info where will auto-cpufreq code be cloned by default as currently your regular user will have no idea about this. Update: I just updated the README with this info, please add anything you think is missing.

Related to this instead of using current working directory of user let's just add this to /opt/auto-cpufreq/source so it will always be there and there won't be changes to dir structure. Especially since install location /opt/auto-cpufreq/venv is also under /opt/auto-cpufreq.

As this will be run with sudo so it'll always end up in in /root/auto-cpufreq about which no one will know about. This also relates to update README section with info about repository structure.

In next iteration it would also be great idea to give users an option to chose where cloned auto-cpufreq dir wil be placed.

I.e: sudo auto-cpufreq --update --dir /home/whatever will place source code to /home/whatever/auto-cpufreq

@cosmos1721
Copy link
Contributor Author

sure, thats a great option to work on.
will add the required info to README section

@cosmos1721
Copy link
Contributor Author

LGTM! Thank you for your contribution, and your perseverance is admired in this process. slightly_smiling_face

You will be credited for your work as part of future release and your future contributors are more then welcome!

thanks a lot

@cosmos1721
Copy link
Contributor Author

Related to this instead of using current working directory of user let's just add this to /opt/auto-cpufreq/source so it will always be there and there won't be changes to dir structure. Especially since install location /opt/auto-cpufreq/venv is also under /opt/auto-cpufreq.

what I'm thinking is to give the user an option in which
if the user only uses auto-cpufreq --update, it will save it to the provided location,but if the user adds --dir, it can work too

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Jul 15, 2023

what I'm thinking is to give the user an option in which
if the user only uses auto-cpufreq --update, it will save it to the provided location,but if the user adds --dir, it can work too

Yes, this is what I also meant in updated comment above. But, if user doesn't specify anything, let's make it so by default it goes to /opt/auto-cpufreq/source (and update README with this info) instead of current working user which in this case will always be /root/auto-cpufreq since --update flag will have to be run with sudo.

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

2 participants