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

Use the defined type to the default value of directory #566

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

y-yagi
Copy link
Contributor

@y-yagi y-yagi commented Jan 5, 2023

Currently, you will get Thor's deprecated message when starting the Listen::CLI.

Deprecation warning: Expected array default value for '--directory'; got "." (string).
This will be rejected in the future unless you explicitly pass the options `check_default_type: false` or call `allow_incompatible_default_type!` in your code
You can silence deprecations warning by setting the environment variable THOR_SILENCE_DEPRECATION.

This is due to the incorrect default value(directory is defined as an array, but the default value is a string).

This fixed to use the correct default value and correctly pass to the directory to Listen.to.

@guard guard deleted a comment from LouisaNikita Jan 6, 2023
@ColinDKelley
Copy link
Collaborator

@y-yagi Can you clarify: when did this warning start? Was this always a bug but just not detected until a recent version of Thor?

@y-yagi
Copy link
Contributor Author

y-yagi commented Jan 6, 2023

@ColinDKelley If my understanding is correct, this warning has started since Thor v1.0.0. PR: rails/thor#626
This isn't a bug and still works well now. But, as the deprecated message describes, it won't work in the future version. So we need to fix it.

Copy link
Collaborator

@ColinDKelley ColinDKelley 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. Just one suggestion on the banner. (I considered changing the option name too, to directories, but that would be an interface change, and the plural case is rare, so it doesn't seem worth it.)

@@ -18,7 +18,7 @@ class CLI < Thor

class_option :directory,
type: :array,
default: '.',
default: ['.'],
aliases: '-d',
banner: 'The directory to listen to'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
banner: 'The directory to listen to'
banner: 'One or more directories to listen to'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review. I fixed.

Currently, you will get Thor's deprecated message when starting the `Listen::CLI`.

```
Deprecation warning: Expected array default value for '--directory'; got "." (string).
This will be rejected in the future unless you explicitly pass the options `check_default_type: false` or call `allow_incompatible_default_type!` in your code
You can silence deprecations warning by setting the environment variable THOR_SILENCE_DEPRECATION.
```

This is due to the incorrect default value(`directory` is defined as an `array`,
but the default value is a `string`).

This fixed to use the correct default value and correctly pass to the `directory`
to `Listen.to`.
@ColinDKelley ColinDKelley merged commit bac8015 into guard:master Jan 9, 2023
@y-yagi y-yagi deleted the fix-thor-deprecation branch January 9, 2023 21:46
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