Skip to content

fix: to_filetype not defined #725

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 29, 2022
Merged

fix: to_filetype not defined #725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c1cc1e
Update tmuxp/cli.py
catroll Jan 8, 2022
844dcca
add some test: convert
catroll Jan 13, 2022
16b3fcb
style: tmuxp/cli.py: width too long (96 > 88)
catroll Jan 18, 2022
21ecc1e
Update tests/test_cli.py
catroll Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,15 +728,18 @@ def test_shell_plus(
(['convert', '.']),
(['convert', '.tmuxp.yaml']),
(['convert', '.tmuxp.yaml', '-y']),
(['convert', '.tmuxp.yml']),
(['convert', '.tmuxp.yml', '-y']),
],
)
def test_convert(cli_args, tmpdir, monkeypatch):
# create dummy tmuxp yaml so we don't get yelled at
tmpdir.join('.tmuxp.yaml').write(
"""
session_name: hello
"""
)
filename = cli_args[1]
if filename == '.':
filename = '.tmuxp.yaml'
file_ext = filename.rsplit('.', 1)[-1]
assert file_ext in ['yaml', 'yml'], file_ext
tmpdir.join(filename).write('\nsession_name: hello\n')
tmpdir.join('.oh-my-zsh').ensure(dir=True)
monkeypatch.setenv('HOME', str(tmpdir))

Expand All @@ -753,6 +756,31 @@ def test_convert(cli_args, tmpdir, monkeypatch):
)


@pytest.mark.parametrize(
"cli_args",
[
(['convert', '.']),
(['convert', '.tmuxp.json']),
(['convert', '.tmuxp.json', '-y']),
],
)
def test_convert_json(cli_args, tmpdir, monkeypatch):
# create dummy tmuxp yaml so we don't get yelled at
tmpdir.join('.tmuxp.json').write('{"session_name": "hello"}')
tmpdir.join('.oh-my-zsh').ensure(dir=True)
monkeypatch.setenv('HOME', str(tmpdir))

with tmpdir.as_cwd():
runner = CliRunner()

# If autoconfirm (-y) no need to prompt y
input_args = 'y\ny\n' if '-y' not in cli_args else ''

runner.invoke(cli.cli, cli_args, input=input_args)
assert tmpdir.join('.tmuxp.yaml').check()
assert tmpdir.join('.tmuxp.yaml').open().read() == 'session_name: hello\n'


@pytest.mark.parametrize("cli_args", [(['import'])])
def test_import(cli_args, monkeypatch):
runner = CliRunner()
Expand Down
9 changes: 7 additions & 2 deletions tmuxp/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1186,10 +1186,15 @@ def command_convert(confirmed, config):
"""Convert a tmuxp config between JSON and YAML."""

_, ext = os.path.splitext(config)
if 'json' in ext:
ext = ext.lower()
if ext == '.json':
to_filetype = 'yaml'
elif 'yaml' in ext:
elif ext in ['.yaml', '.yml']:
Copy link
Member

Choose a reason for hiding this comment

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

This looks good!

Can we have a test for it? (e.g. in https://github.com/tmux-python/tmuxp/blob/master/tests/test_cli.py#L725-L733)

Copy link
Member

Choose a reason for hiding this comment

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

@catroll at your request I can also merge this and I can write the test after merge. Whichever works best for you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I write some test first?
But I'm not good enough at pytest, so maybe need some time to learn it.

Copy link
Member

Choose a reason for hiding this comment

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

I will give you the choice, since it's an opportunity to learn. I can wait.

If you'd like to learn and write it yourself, the instructions here:

As an alternative, if that's too much effort for a fix of this size, please let me know if you'd like to skip writing a test this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will write test myself, after work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i write some tests
please review it
is it ok?
@tony

Copy link
Member

Choose a reason for hiding this comment

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

@catroll

I ran the tests, I will take a look evening time (by me)

P.S. To avoid merge commits, you can do real rebase, e.g.

In your branch:

git remote add tmux-python https://github.com/tmux-python/tmuxp.git
git pull --rebase tmux-python master
git push --force-with-lease

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

to_filetype = 'json'
else:
raise click.BadParameter(
'Unknown filetype: %s (valid: [.json, .yaml, .yml])' % (ext,)
)

configparser = kaptan.Kaptan()
configparser.import_config(config)
Expand Down