-
Notifications
You must be signed in to change notification settings - Fork 45
Overwrite ability #106
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
Overwrite ability #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The implementation looks good to me; let's figure out the tests.
Also:
--overwrite ability will work only with --single-version argument
Can we display a warning if --overwrite
is specified but --single-version
is missing?
CHANGELOG.md
Outdated
@@ -2,6 +2,12 @@ | |||
|
|||
## master | |||
|
|||
## 0.15.4 (2023-01-05) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not update versions in PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to ##master
README.md
Outdated
@@ -219,6 +219,7 @@ Usage: ruby-next nextify DIRECTORY_OR_FILE [options] | |||
-o, --output=OUTPUT Specify output directory or file or stdout | |||
--min-version=VERSION Specify the minimum Ruby version to support | |||
--single-version Only create one version of a file (for the earliest Ruby version) | |||
--overwrite Overwrites the original file with one version of --single-version (Works with --single-version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--overwrite Overwrites the original file with one version of --single-version (Works with --single-version) | |
--overwrite Overwrites the original file with one version of --single-version (works only with --single-version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to
**works only with --single-version or --rewrite**
lib/ruby-next/commands/nextify.rb
Outdated
@@ -210,6 +236,10 @@ def stdout? | |||
def single_version? | |||
single_version || specified_rewriters | |||
end | |||
|
|||
def overwrite_original_file? | |||
single_version && overwrite_original_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove single_version
from here, since we explicitly check for it presence after parsing
single_version && overwrite_original_file | |
overwrite_original_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked.
lib/ruby-next/commands/nextify.rb
Outdated
@@ -108,6 +113,11 @@ def parse!(args) | |||
exit 2 | |||
end | |||
|
|||
if @overwrite_original_file && !@single_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we should use single_version?
here and also mention --rewriters
(as an alternative to --single-version
) in the message:
if @overwrite_original_file && !@single_version | |
if overwrite_original_file? && !single_version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
done
Also added spec test for --overwrite and --rewrite args
…s. Add spec overwrite_original_file with --rewrite
Hello @palkan |
…ngPatternError, so we need load -ruby-next)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help!
I'll merge this and apply changes myself.
old_content = File.read(File.join(__dir__, "dummy", "transpile_me.rb")) | ||
File.write(File.join(__dir__, "dummy", "overwrite", "transpile_me.rb"), old_content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can prepare a fixture in the before
hook instead of restoring it in the after
hook. This way, we can avoid duplicating the transpile_me.rb
file.
@@ -119,6 +124,11 @@ def parse!(args) | |||
end | |||
end | |||
|
|||
if overwrite_original_file? && !single_version? | |||
$stdout.puts "--single-version and --rewrite are missing, --overwrite arg works only with --single-version or --rewrite" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phrase "--single-version and --rewrite are missing" is confusing; it sounds like we need both
Hello, @palkan!
Please, review my changes
What is the purpose of this pull request?
Support passing --overwrite to CLI for overwriting the original_file
What changes did you make? (overview)
Add the ability to parse --overwrite
Is there anything you'd like reviewers to focus on?
--overwrite
ability will work only with--single-version
argument. Without the argument--single-version
overwrite mode will ignoredChecklist
Closes #104