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 Application config menu in About section. #821

Closed
wants to merge 2 commits into from

Conversation

zee-bit
Copy link
Member

@zee-bit zee-bit commented Nov 4, 2020

The about section has now an added menu Application Configuration, that lists the options that Zulip-terminal was loaded with. Here is the screenshot of the new About section:

About-section-zt

Fixes #812

@zulipbot zulipbot added the size: S [Automatic label added by zulipbot] label Nov 4, 2020
@zulipbot
Copy link
Member

zulipbot commented Nov 4, 2020

Hello @zee-bit, it seems like you have referenced #812 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #812..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to Zulip!

 Author:    zee-bit
 Date:      Wed Nov 4 10:49:49 2020 +0530
 Pull Request: WIP: Add Application config menu in About section.

 Fixes zulip#51
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.

@zee-bit Thanks for working on this 👍 The code change looks reasonable and works fine :)

It would be good to fix the tests - you can try running pytest like I mentioned in the issue. The errors are in the core and popups tests, and it looks like a lot, but you should see that many of these are duplicates. You may find the -x (fail after any failure) and --lf (just run recent last-failing tests) options useful.

In addition, while the commit title text is fine, if you take a look at our development notes in the README or browse our git history, you'll see that we have a different style, which it would be great if you could adapt the commit to 👍 I'm not sure why you have the commit summary text you have - we can discuss that or anything else here or in the #zulip-terminal stream if you like.

@@ -28,9 +28,10 @@ class Controller:
the application.
"""

def __init__(self, config_file: str, theme: ThemeSpec,
def __init__(self, config_file: str, theme_name: str, theme: ThemeSpec,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you've added an extra parameter to this method, this has broken the tests which expect a different number of parameters.

Comment on lines +164 to +167
server_feature_level=self.model.server_feature_level,
theme_name=self.theme_name, color_depth=self.color_depth,
autohide_enabled=self.autohide,
footlink_enabled=self.footlinks_enabled)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The extra parameters added here also have broken the AboutView tests, so you'll need to update these too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't able to think of a way to access these parameters in test_popups so, I just hard-coded them. I think for testing purposes this should be okay, as we just need to check the parameters given are fine and with desired types. I also looked at other classes from the test_popup file and I found some similar cases there as well.

@neiljp neiljp added the enhancement New feature or request label Nov 4, 2020
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Nov 4, 2020
@zee-bit zee-bit closed this Nov 5, 2020
@zee-bit zee-bit reopened this Nov 5, 2020
@neiljp neiljp added this to the Next Release milestone Nov 6, 2020
@neiljp
Copy link
Collaborator

neiljp commented Nov 6, 2020

@zee-bit Thanks for working on this 👍 I squashed your commits together to avoid an intermediate state where the tests were broken and have reworded them to match our style, as per my earlier review. I've pushed that commit as 24519ac 🎉

@neiljp neiljp closed this Nov 6, 2020
@zee-bit
Copy link
Member Author

zee-bit commented Nov 6, 2020

@neiljp Thanks for the review and those handy pytest flags, they helped a lot. As for the commit summary, I'll try to adapt myself to the recommended way asap. Looking forward to work on more issues 💯

@neiljp
Copy link
Collaborator

neiljp commented Nov 6, 2020

@zee-bit Thanks for the contribution 👍 We've discussed adding pytest tips to the docs previously, so I've added some to #820 at https://github.com/neiljp/zulip-terminal/tree/2020-11-03-FAQ-updates#tips-for-working-with-tests-pytest

If you have any feedback, please let us know :)

@zee-bit
Copy link
Member Author

zee-bit commented Nov 7, 2020

@neiljp This is a very nice addition. Although the list of commands is more than enough for testing ZT code-changes, it would be better if we could add the link to official pytest docs there too.

@zee-bit zee-bit changed the title WIP: Add Application config menu in About section. Add Application config menu in About section. Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size: M [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show configuration options for current session in about popup
3 participants