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

refactor/enhancement: Added "[default theme]" suffix to the --list-themes command-line option and cleaned up the duplicate code. #807

Closed
wants to merge 1 commit into from

Conversation

sumagnadas
Copy link
Collaborator

@sumagnadas sumagnadas commented Oct 12, 2020

This PR adds the suffixes [default theme] to the listed themes. The first suffix is applied to the theme zt_dark which is the default theme for Zulip Terminal.

This PR also combines the two places with a function which lists themes hence cleaning up the duplicate code present because of the addition of the --list-themes command-line option.

A follow up to PR #803.

@neiljp neiljp added the PR needs review PR requires feedback to proceed label Oct 12, 2020
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@sumagnadas Thanks for the follow-up. As below, I've merged one commit with minor adjustments already, but it would be useful to not have a valid config file in order just to list themes.

zulipterminal/cli/run.py Outdated Show resolved Hide resolved
docs/FAQ.md Outdated Show resolved Hide resolved
@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Oct 17, 2020
@sumagnadas
Copy link
Collaborator Author

I kept the user_preference suffix to denote the theme which is either specified on the command-line or the zuliprc config file. So its there because of the command-line option specification.

@sumagnadas
Copy link
Collaborator Author

I also removed the FAQ commit to squash the initial commit and the "code refactor" commit into one.

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@sumagnadas Thanks for taking the function approach. This is looking closer to the use of common code for --list-themes and the 'bad theme' case now that I had envisaged in an earlier response 👍

With the current way that the rest of the code is structured in this file - the prompting for server details and so on being inside other functions, if the configuration file isn't found to be valid - then I think we should leave the has_config and specified-theme parameters for now:

  • --list-themes being above, can't have this information without adjusting the way these other functions work
  • in the case of an invalid theme, then passing the theme won't achieve anything, as by definition it is invalid!

So, we can still use the new function at the top (just for args.list_themes) and below (just for the invalid theme), but completely separately for now.

zulipterminal/cli/run.py Outdated Show resolved Hide resolved
@sumagnadas
Copy link
Collaborator Author

I also edited the first commit to not include the [user preference] because, in the second commit, it was removed.

@sumagnadas
Copy link
Collaborator Author

@neiljp Anything I should change in any of the commits?

@neiljp
Copy link
Collaborator

neiljp commented Oct 27, 2020

@sumagnadas I think it'd work best with the two commits squashed together with a reworded commit text. The overall code looks good in the 'files changed' tab of the PR, and the commit text can reflect that 👍

My reasoning for suggesting squashing is based on the overall result of this PR in the 'files changed' tab - each current commit doesn't seem to build towards it.

@sumagnadas sumagnadas changed the title enhancement: Added suffixes to the --list-themes command-line option. refactor/enhancement: Added default suffix to the --list-themes command-line option and cleaned up the duplicate code. Oct 27, 2020
@sumagnadas sumagnadas changed the title refactor/enhancement: Added default suffix to the --list-themes command-line option and cleaned up the duplicate code. refactor/enhancement: Added "[default theme]" suffix to the --list-themes command-line option and cleaned up the duplicate code. Oct 27, 2020
This adds a function, which outputs a list of themes on the screen,
to remove code duplication. This also adds the suffix "[default theme]"
at the end of the theme "zt_dark" in the theme listing.
@sumagnadas
Copy link
Collaborator Author

See if this works. Sorry if the commit text is not appropriate. I am still trying to get the hang of working with Github repos, commits and the other stuff of open source contribution.

@neiljp neiljp added enhancement New feature or request hacktoberfest-accepted This PR is considered valid for Hacktoberfest :) labels Oct 27, 2020
@neiljp
Copy link
Collaborator

neiljp commented Oct 27, 2020

@sumagnadas Thanks for working on this 👍 I've slightly reworded the commit text and merged it otherwise as-is 🎉

If you want to see the differences in the commit title/description and aren't sure how to go about that, I'd recommend learning about git range-diff.

I also merged #810 - if you have any questions or other feedback on if that would have helped with writing the commits, we'd be happy to know :) (see mainly the GitLint section)

@neiljp neiljp closed this Oct 27, 2020
@neiljp neiljp added this to the Next Release milestone Oct 27, 2020
@sumagnadas sumagnadas deleted the pr803-follow-up branch June 3, 2021 17:58
@sumagnadas sumagnadas restored the pr803-follow-up branch June 3, 2021 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest-accepted This PR is considered valid for Hacktoberfest :) PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback size: S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants