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

Custom configs #62

Merged
merged 4 commits into from Sep 4, 2020
Merged

Custom configs #62

merged 4 commits into from Sep 4, 2020

Conversation

jonnieey
Copy link
Contributor

Simple custom binds and color configuration from file.

@pawamoy
Copy link
Owner

pawamoy commented Jun 22, 2020

Yay! Thank you very much for your contribution, I'll take the time to review properly the next days.

I see however that you pushed already merged commits? What happened? Could you try to rebase your branch on master to resolve the merge conflicts?

@jonnieey
Copy link
Contributor Author

jonnieey commented Jun 22, 2020 via email

@pawamoy
Copy link
Owner

pawamoy commented Jun 22, 2020

No problem, I'll do my best to help you!

In your branch, the commands should be

git rebase master
# fix any conflict, follow what git is telling you
# now you can "force push" to your branch
git push -f

Please don't hesitate to ask if something is unclear :)

@pawamoy
Copy link
Owner

pawamoy commented Jun 22, 2020

Oh, I think I can also push to your branch. I'll try to do it later if you didn't do it first ^^

@pawamoy
Copy link
Owner

pawamoy commented Jun 22, 2020

Don't worry too much about CI (quality and tests jobs), they were already failing before.

@pawamoy
Copy link
Owner

pawamoy commented Jul 7, 2020

Hey @jonnieey, I didn't forget about your PR, I simply was very busy these last weeks (moving out / moving in, work) and I want to review your PR properly, which require a decent amount of time 🙂 But I'll get to it eventually! Sorry it's taking so long for your first contribution 😄

Copy link
Owner

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Thanks again @jonnieey, these are great additions 🙂

I would have preferred a more modern configuration format like YAML/TOML, but I think the INI format is OK to start with.

One other thing that bothers me is that we can only have one key for an action now? We could previously use both q and F1 to display help, and now it's only F1?

src/aria2p/api.py Show resolved Hide resolved
src/aria2p/api.py Outdated Show resolved Hide resolved
src/aria2p/api.py Outdated Show resolved Hide resolved
src/aria2p/interface.py Outdated Show resolved Hide resolved
src/aria2p/interface.py Outdated Show resolved Hide resolved
src/aria2p/interface.py Outdated Show resolved Hide resolved
src/aria2p/utils.py Outdated Show resolved Hide resolved
src/aria2p/utils.py Outdated Show resolved Hide resolved
src/aria2p/utils.py Outdated Show resolved Hide resolved
src/aria2p/utils.py Outdated Show resolved Hide resolved
@jonnieey jonnieey force-pushed the custom-configs branch 2 times, most recently from 24bd2f6 to 69b388f Compare August 1, 2020 15:50
@jonnieey jonnieey closed this Aug 1, 2020
@jonnieey jonnieey deleted the custom-configs branch August 1, 2020 21:27
@jonnieey jonnieey restored the custom-configs branch August 1, 2020 21:27
@jonnieey jonnieey reopened this Aug 1, 2020
jonnieey added a commit to jonnieey/aria2p that referenced this pull request Aug 2, 2020
@jonnieey jonnieey force-pushed the custom-configs branch 2 times, most recently from ca610aa to c4bef36 Compare August 3, 2020 11:53
@pawamoy
Copy link
Owner

pawamoy commented Sep 3, 2020

Hello @jonnieey, I commited some changes:

  • create the configuration file if it does not exist, write the default configuration values
  • remove config.ini
  • add the vim-keybindings in the default config

How do you feel about this PR? I would like to merge it and continue integrating it properly (like adding tests, fixing CI, etc.)

@jonnieey
Copy link
Contributor Author

jonnieey commented Sep 3, 2020

Hey @pawamoy. I think it's a good start for custom user configurations. There's room for a lot of improvement that can be made along the way.

@pawamoy
Copy link
Owner

pawamoy commented Sep 4, 2020

@jonnieey alright great! I'll squash and merge. Feel free to send more PRs 😉

@pawamoy pawamoy merged commit 9d6b00f into pawamoy:master Sep 4, 2020
pawamoy added a commit that referenced this pull request Sep 4, 2020
@pawamoy
Copy link
Owner

pawamoy commented Sep 4, 2020

Oops, your authoring was dropped from the commits, let me fix this.

@pawamoy
Copy link
Owner

pawamoy commented Sep 4, 2020

@jonnieey I can't find back your git author name and email, can you give them to me 🙂 ?

I insist.

@pawamoy
Copy link
Owner

pawamoy commented Sep 4, 2020

Nevermind, found it!

pawamoy added a commit that referenced this pull request Sep 4, 2020
Fixes #60.
PR #62.

Co-authored-by: Timothée Mazzucotelli <pawamoy@pm.me>
@pawamoy
Copy link
Owner

pawamoy commented Sep 4, 2020

Sorry for this mess 😅 It's fixed!

@jonnieey jonnieey deleted the custom-configs branch October 9, 2020 06:12
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