Skip to content

Setter for TextArea load_text#3545

Merged
darrenburns merged 1 commit intoTextualize:mainfrom
JEFuller:main
Oct 26, 2023
Merged

Setter for TextArea load_text#3545
darrenburns merged 1 commit intoTextualize:mainfrom
JEFuller:main

Conversation

@davetapley
Copy link
Copy Markdown
Contributor

@davetapley davetapley commented Oct 16, 2023

As discussed #3525 (reply in thread)

Please review the following checklist.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@davetapley
Copy link
Copy Markdown
Contributor Author

Thoughts on tests / doc changes? 🤔

Copy link
Copy Markdown
Contributor

@TomJGooding TomJGooding left a comment

Choose a reason for hiding this comment

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

I'm not a maintainer of Textual, but your code is missing type annotations and I've suggested tweaks to the docstring.

Do you think you could also add a simple test?

Comment thread src/textual/widgets/_text_area.py Outdated
Comment thread src/textual/widgets/_text_area.py Outdated
@davetapley
Copy link
Copy Markdown
Contributor Author

Thanks @TomJGooding, I wasn't sure about the argument doc string for a setter, but I guess there's no harm.

And r.e. tests it seems needlessly duplicitous when we already have load_text covered.
I suppose I could test that the setter calls that with a mock, but that's not really testing anything 🤪

@TomJGooding
Copy link
Copy Markdown
Contributor

Sorry you might be right with this only being a setter. I think I was just on auto-pilot (and my change upset black).

I will mind my own business and leave this for the maintainers!

@darrenburns
Copy link
Copy Markdown
Member

darrenburns commented Oct 17, 2023

A simple test is still valuable as a guard against future regressions - it's a simple setter for now but could be modified in the future, so we want to capture it's expected behaviour. Just something like this would suffice:

new_text = "hello\nworld\n"
text_area.text = new_text
assert text_area.text == new_text

@darrenburns
Copy link
Copy Markdown
Member

I wrote this test here for it, but I think your fork is locked as I can't push to this PR:

async def test_text_setter():
    app = TextAreaApp()
    async with app.run_test():
        text_area = app.query_one(TextArea)
        new_text = "hello\nworld\n"
        text_area.text = new_text
        assert text_area.text == new_text

Also, if you could add a CHANGELOG.md entry too when you get a moment, that'd be great.

@davetapley davetapley marked this pull request as ready for review October 25, 2023 22:12
Copy link
Copy Markdown
Member

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@darrenburns darrenburns merged commit 41cadfc into Textualize:main Oct 26, 2023
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.

3 participants