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

feat: allow very_good create . #996

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

a-wallen
Copy link

@a-wallen a-wallen commented Mar 19, 2024

Status

READY/IN DEVELOPMENT/HOLD
READY

Description

Allows . as a project name (like flutter create .)

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@a-wallen a-wallen requested a review from a team as a code owner March 19, 2024 07:42
@a-wallen
Copy link
Author

fixes #969

@a-wallen
Copy link
Author

this is ready for review.

by the way, the repo had 37 broken tests when I cloned it on my machine. I was wondering if I should submit those as issues.

@tomarra
Copy link
Contributor

tomarra commented Mar 27, 2024

Fixed the formatting issue by updating from main via #1003 so CI is now passing. @wolfenrain can we get a review here?

Comment on lines 124 to 141
/// The project name that the user provided.
///
/// Since the project name could be user provided as either:
/// 1. A valid package name
/// 2. '.', the current working directory (the project assumes the cwd's name)
/// this needs to exist to provide a fallback value for [outputDirectory] and
/// [projectName].
String get _projectName {
final args = argResults.rest;
_validateProjectName(args);
return args.first;
}

/// Gets the project name.
String get projectName => _projectName == '.'
? path.basename(Directory.current.path)
: _projectName;

Copy link
Member

Choose a reason for hiding this comment

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

I think we should rework this. Introducing . as a projectName and using it to find the name of the directory is kinda confusing because what if I pass path/to/my/folder instead?

. implies current directory which implicitly also means we can pass any path, which isnt the case. However this is the case for flutter create so maybe we should just make it that we can pass any given path and use the last part of that path as the projectName and create the template at the given path.

What you think?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @wolfenrain, thanks for the positive feedback.

I agree, let's do it the same way as the Flutter tool.

This means we'd remove --output-directory as an argument.

As far as very_good create <arg>, here are some requirements that I could implement:

  • Allow relative paths as arg.
  • Allow absolute paths as arg.
  • Use the last path segment of arg as the project name.
  • Remove the --output-directory argument.

Let me know if you'd like me to proceed. Thanks for catching my oversight 👍

Copy link
Member

Choose a reason for hiding this comment

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

I do think that is a good approach, so arg would be any type of path indeed, it would be nice if we can keep backwards compatibility with the --output-directory. Maybe by just checking if it is supplied, if it is show that it is deprecated and just take that over whatever was in arg, assuming arg was a path?

PS: it would be very_good create <template> <output directory>

Copy link
Member

Choose a reason for hiding this comment

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

Hey @a-wallen just following up here to see if you are planning on tackling this still?

@a-wallen a-wallen force-pushed the allow_create_in_cwd branch from 89ded61 to b706a2d Compare July 9, 2024 23:37
Comment on lines +120 to +128
final directory = argResults['output-directory'] as String?;

if (directory != null) {
return Directory(directory);
}

final args = argResults.rest;

return Directory(args.first);
Copy link
Author

Choose a reason for hiding this comment

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

same logic as before, if the --output-directory argument isn't provided then treat the [project-name] argument as as a directory path.

@alestiago alestiago requested a review from wolfenrain July 10, 2024 09:43
@alestiago
Copy link
Contributor

Pinging @wolfenrain

@tomarra tomarra added the p2 Important issues not at the top of the work list label Aug 30, 2024
Copy link
Member

@wolfenrain wolfenrain left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the late follow-up on my part. I will try and test out this branch later today to make sure it is backwards compatible but from the looks of it we should be good!

@a-wallen
Copy link
Author

sure thing, let me know if there's anything I need to do.

@wolfenrain
Copy link
Member

sure thing, let me know if there's anything I need to do.

The CI seems to be failing on some tests, mind having a look at that? I suspect that the tests are failing because of wrong parameters (might mean it isnt backwards compatible like we hoped?)

@a-wallen
Copy link
Author

a-wallen commented Oct 9, 2024

I can't tell which tests are failing. See this issue for more details #1162

@tomarra
Copy link
Contributor

tomarra commented Oct 10, 2024

I can't tell which tests are failing. See this issue for more details #1162

Take a look at the details of the Actions output here. You can see the failing tests in the output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 Important issues not at the top of the work list
Projects
Status: Community
Development

Successfully merging this pull request may close these issues.

4 participants