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

Alternative method to initialise Select #3691

Closed
rodrigogiraoserrao opened this issue Nov 16, 2023 · 9 comments · Fixed by #3743
Closed

Alternative method to initialise Select #3691

rodrigogiraoserrao opened this issue Nov 16, 2023 · 9 comments · Fixed by #3743
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Nov 16, 2023

I propose we extend Select.__init__ or create a classmethod Select.from_values to accept an iterable of SelectType and automatically build the labels by converting them to strings.

I find that often we just want labels that are string representations of the values we want to select:

Select.from_values(data_source)
# vs
Select([(str(datum), datum) for datum in data_source])

data_source is often just a list of strings or paths and the comprehension to create the tuple looks really awkward.

For example, in the Select docs we do this:

# ...
yield Select((line, line) for line in LINES)

I personally don't like it at all.

@rodrigogiraoserrao rodrigogiraoserrao added enhancement New feature or request question Further information is requested labels Nov 16, 2023
@azinneck0485
Copy link
Contributor

Can I work on this?

I have done a quick prototype solution following the class method approach you suggested. Before I go forward with formalizing the prototype, I wanted to ask: is this preferrable to overloading the constructor?

@rodrigogiraoserrao
Copy link
Contributor Author

Hey @azinneck0485 thanks for your interest!
I don't see any particular reason why you wouldn't be able to work on this. 🤷

Let us just make sure that the class method is a good idea and then we'll let you know.
If you're working on this, make sure to read the contributing guidelines for an experience as smooth as possible!

@willmcgugan
Copy link
Collaborator

I think we should go ahead and add the classmethod. @azinneck0485 are you happy to contribute that?

@azinneck0485
Copy link
Contributor

Yes, I will contribute that change and get a PR in once I've tested it.

@rodrigogiraoserrao
Copy link
Contributor Author

Excellent @azinneck0485!
I've assigned this to you. (I've also assigned it to me for bookkeeping purposes.)
Take your time and then make sure to ping me once your PR is in!

@azinneck0485
Copy link
Contributor

Hi @rodrigogiraoserrao -- I have made the code changes and done some testing on my own. I have also updated the documentation by adding an example (which is just the existing example modified to use the new class method to create the select control). All changes have been pushed to a branch on my fork.

However, I am not yet ready for a PR as I have run into some problems running pytest. Although I have only added 3 tests (replicating the existing tests for Select()), 16 tests are failing in total. 13 of those failing tests are from test_text_area_language_rendering and test_text_area_themes and are due to missing syntax highlighting in the tests I have run.

I don't want to accidentally update snapshots erroneously, so I wanted to check and see if you had any tips on what I should look into to ensure these tests pass?

@rodrigogiraoserrao
Copy link
Contributor Author

Hi @azinneck0485, thanks for the update!
If you're running Python 3.7, you won't be able to install the dependencies tree-sitter and tree_sitter_languages, which require Python 3.8 or greater.
If you're running Python 3.8 or greater, make sure you have the dependencies installed.

In my case, I'm running Python 3.7 and if I run make test, this is what the final output looks like:

Screenshot 2023-11-23 at 11 01 48

Notice how there's a red warning saying there are 13 unused snapshots, which makes it look like in the end the tests failed (see the Error 1 at the bottom).

However, at the same time, pytest reports that those tests were skipped.

In short, there's an issue with the snapshot tests reporting (which is a problem in the tool we use for the snapshots) and it's ok for those 13 snapshots to report that “error”.

In order to update only the snapshot(s) you created, first make sure they look ok.
(I.e., run the test, open the snapshot report error, and make sure the snapshot looks like it is intended to look.)
Then, run the test again with the flag --snapshot-update, but only on your tests.

For example, if you had a snapshot test called test_azinneck_snap, you could run pytest -vv tests/snapshot_tests/test_snapshots.py::test_azinneck_snap --snapshot-update and it would update only your snapshot.

@azinneck0485
Copy link
Contributor

@rodrigogiraoserrao, thanks for you tips! After adding those packages, the failures went away and I was able to successfully run the new snapshot tests I created.

I have added PR #3743 that includes the code change, test changes, and documentation changes. Please let me know if there are additional changes that I need to make.

Thanks again for all your help.

Copy link

github-actions bot commented Dec 5, 2023

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants