Skip to content

Projectfile #39

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

Merged
merged 25 commits into from
Sep 27, 2017
Merged

Projectfile #39

merged 25 commits into from
Sep 27, 2017

Conversation

amjith
Copy link
Contributor

@amjith amjith commented Sep 23, 2017

Description

This will make the init command and the new command interactive.

Checklist

@amjith
Copy link
Contributor Author

amjith commented Sep 23, 2017

I'll fix the conflicts and make the tests pass.

@ofek
Copy link
Collaborator

ofek commented Sep 23, 2017

Thanks! I'll look this over later after I fix master. My initial impression though is I would prefer to enable interactivity with an -i/--interactive flag. Only being interactive makes the config file way less useful and as a user I would want non-interactive by default.

@amjith
Copy link
Contributor Author

amjith commented Sep 23, 2017

I think you make a good point. Except the package name and the description all the other questions are pretty static and it can be supplied by the config file.

I'll change the behavior to add a -i option for interactive.

@amjith
Copy link
Contributor Author

amjith commented Sep 24, 2017

The tests are now passing. But I haven't added new tests to cover the code that I have added. I'll work on adding tests tomorrow. In the meantime, I welcome any comments or suggestions.

@amjith
Copy link
Contributor Author

amjith commented Sep 25, 2017

I've added tests to the interactive init command. I have also fixed the failing tests. The PR is pretty big. I'm happy to do a screen share and walk you through the main points if you're interested.

I'll add more tests to the interactive new command as well, but I think this PR is ready for your review.

@@ -14,7 +14,7 @@

@click.command(context_settings=CONTEXT_SETTINGS,
short_help='Creates a new Python project in the current directory')
@click.argument('name')
@click.argument('name', nargs=-1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

required=False

@@ -15,7 +15,7 @@

@click.command(context_settings=CONTEXT_SETTINGS,
short_help='Creates a new Python project')
@click.argument('name')
@click.argument('name', nargs=-1, default=None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

required=False

package_name = name[0]

if interactive or not name:
pname = os.path.split(cwd)[-1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

use basepath from hatch.utils

shell=NEED_SUBPROCESS_SHELL)
return email.strip().decode('utf-8')
except:
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea! Just return instead of return None and add # no cov to each function definition.

@@ -19,8 +20,8 @@
('build', ''),
])),
('pypi_username', ''),
('name', 'U.N. Owen'),
('email', '[email protected]'),
('author', get_user() or 'U.N. Owen'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the decision here, but can we please keep it as before? It would be a breaking change.

@ofek
Copy link
Collaborator

ofek commented Sep 25, 2017

In init and new command files can you please change from hatch.settings import load_settings to from hatch.settings import copy_default_settings, load_settings and use that new function in the except FileNotFoundError:? That was an oversight of mine.

You can then use the default settings in the functions.

@ofek
Copy link
Collaborator

ofek commented Sep 25, 2017

@ofek
Copy link
Collaborator

ofek commented Sep 25, 2017

pypa/pip#4144 (comment)

@@ -25,7 +25,7 @@ def test_name():
with temp_chdir() as d:
settings = copy_default_settings()
settings['licenses'] = ['apache2']
settings['name'] = 'Don Quixote'
settings['author'] = 'Don Quixote'
Copy link
Collaborator

Choose a reason for hiding this comment

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

here too

@@ -25,7 +25,7 @@ def test_name():
with temp_chdir() as d:
settings = copy_default_settings()
settings['licenses'] = ['cc0']
settings['name'] = 'Guy Fawkes'
settings['author'] = 'Guy Fawkes'
Copy link
Collaborator

Choose a reason for hiding this comment

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

here too

@@ -25,7 +25,7 @@ def test_name():
with temp_chdir() as d:
settings = copy_default_settings()
settings['licenses'] = ['mit']
settings['name'] = 'Don Quixote'
settings['author'] = 'Don Quixote'
Copy link
Collaborator

Choose a reason for hiding this comment

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

here too

@@ -25,7 +25,7 @@ def test_name():
with temp_chdir() as d:
settings = copy_default_settings()
settings['licenses'] = ['mpl']
settings['name'] = 'Red Panda'
settings['author'] = 'Red Panda'
Copy link
Collaborator

Choose a reason for hiding this comment

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

here too

@ofek
Copy link
Collaborator

ofek commented Sep 26, 2017

Sorry, I missed a few.

@ofek
Copy link
Collaborator

ofek commented Sep 26, 2017

Looks great! We just need to change the pyproject.toml: pypa/pip#4144 (comment) until end of thread

So, initially I was thinking we could have metadata and commands, but exclude packages for when Hatch supports Pipfiles. Sound good?

@amjith
Copy link
Contributor Author

amjith commented Sep 26, 2017

Looks like one test is failing on one Python36-x64 Appveyor.

Looks to me it's a timeout failure. Can you take a look?

@ofek
Copy link
Collaborator

ofek commented Sep 26, 2017

That's just virtualenv being slow, it happens sometimes :/


if os.path.exists(d):
echo_failure('Directory `{}` already exists.'.format(d))
sys.exit(1)

if licenses:
settings['licenses'] = map(str.strip, licenses.split(','))
Copy link
Collaborator

Choose a reason for hiding this comment

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

In init this comes after the interactive branch. What do you want to give precedence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user runs this:

hatch init -i -l 'mit,mpl'

Hatch will use the mit,mpl as the default value during the interactive mode. If the user chooses to override it during the interactive mode then the overridden value from the interactive mode will be used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now the thing is that in new the if licenses comes before the if interactive but in init it's the opposite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. They were functionally equivalent. But I've changed the code to match the same pattern in both files.

runner.invoke(hatch, ['init', '-i', '--basic', '-ne'], input='ok\n0.1.0\nTest Description\nPicard\[email protected]\nmpl\n')

assert os.path.exists(os.path.join(d, 'ok', '__init__.py'))
assert "__version__ = '0.1.0'\n" == open(os.path.join(d, 'ok', '__init__.py')).read()
Copy link
Collaborator

Choose a reason for hiding this comment

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

runner.invoke(hatch, ['new', '-i', '--basic', '-ne'], input='ok\n0.1.0\nTest Description\nPicard\[email protected]\nmpl\n')

assert os.path.exists(os.path.join(d, 'ok', 'ok', '__init__.py'))
assert "__version__ = '0.1.0'\n" == open(os.path.join(d, 'ok', 'ok', '__init__.py')).read()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ofek
Copy link
Collaborator

ofek commented Sep 27, 2017

The PR looks great except the pyproject.toml needs:

[build-system]
requires = ["setuptools", "wheel"]

and put metadata right under [tool.hatch].

@ofek
Copy link
Collaborator

ofek commented Sep 27, 2017

Tests failing due to source -> metadata change.

@ofek ofek merged commit 9cf614f into pypa:master Sep 27, 2017
@amjith
Copy link
Contributor Author

amjith commented Sep 27, 2017

WooHoo!! 🎉

@amjith amjith deleted the projectfile branch September 27, 2017 20:36
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.

2 participants