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

Default to check the entire current directory when there is no Steepfile #968

Merged
merged 5 commits into from
Dec 27, 2023
Merged

Default to check the entire current directory when there is no Steepfile #968

merged 5 commits into from
Dec 27, 2023

Conversation

ParadoxV5
Copy link
Contributor

@ParadoxV5 ParadoxV5 commented Nov 24, 2023

This allows Steep to work out-of-the-box with no further setup required.
Though of course users are encouraged to set up their Steepfile according to their project structure.

May relate to soutaro/steep-vscode#227 (comment)

Tests

  • Integration Test: no idea where to put it <.<
    • DriverHelper: no unit test
    • Project: empty unit test
    • WorkerProcess: no unit test

Q: Why current directory (rather than lib + sig)?

This matches the minimum (i.e., least set up) project structure of dumping everything in the project root.

Recommended follow-up

  • Utilize nilable Steepfile path in tests instead of leaving the Steepfile non-existent.

@@ -21,7 +21,7 @@ def initialize(stdout:, stderr:, stdin:)

def run()
Steep.logger.tagged("#{worker_type}:#{worker_name}") do
project = load_config()
project = load_config(warnings: false) # Main process has already emitted any warnings
Copy link
Contributor Author

@ParadoxV5 ParadoxV5 Nov 24, 2023

Choose a reason for hiding this comment

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

Unfortunately, Steep spawns entirely new worker processes (likely considering the Ruby GVL and that Windows has no fork) with no parent-child relationship to the main process. This means every worker have to completely reload Steepfile.

Steps to Reproduce

$ mkdir steep_workers
$ cd steep_workers
$ echo "puts 'hello, world'" > Steepfile
$ steep check

Actual

hello, world
# Type checking files:

hello, world
hello, world
hello, world
hello, world
hello, world
hello, world
hello, world
hello, world


No type error detected. 🫖

Expected

hello, world
# Type checking files:



No type error detected. 🫖

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Steps to Reproduce

$ mkdir steep_workers
$ cd steep_workers
$ echo 'target :app do typing_options end' > Steepfile
$ steep check

Actual

[Steep 1.6.0] [Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
# Type checking files:

[Steep 1.6.0] [typecheck:typecheck@0] [/tmp/steep2/steep_workers/Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [typecheck:typecheck@0] [/tmp/steep2/steep_workers/Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
[Steep 1.6.0] [typecheck:typecheck@1] [/tmp/steep2/steep_workers/Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [typecheck:typecheck@1] [/tmp/steep2/steep_workers/Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
[Steep 1.6.0] [typecheck:typecheck@2] [/tmp/steep2/steep_workers/Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [typecheck:typecheck@2] [/tmp/steep2/steep_workers/Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
[Steep 1.6.0] [typecheck:typecheck@3] [/tmp/steep2/steep_workers/Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [typecheck:typecheck@3] [/tmp/steep2/steep_workers/Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
[Steep 1.6.0] [typecheck:typecheck@4] [/tmp/steep2/steep_workers/Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [typecheck:typecheck@4] [/tmp/steep2/steep_workers/Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
[Steep 1.6.0] [typecheck:typecheck@5] [/tmp/steep2/steep_workers/Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [typecheck:typecheck@5] [/tmp/steep2/steep_workers/Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
[Steep 1.6.0] [typecheck:typecheck@6] [/tmp/steep2/steep_workers/Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [typecheck:typecheck@6] [/tmp/steep2/steep_workers/Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
[Steep 1.6.0] [typecheck:typecheck@7] [/tmp/steep2/steep_workers/Steepfile] [target=app] #typing_options is deprecated and has no effect as of version 0.46.0. Update your Steepfile as follows for (almost) equivalent setting:
[Steep 1.6.0] [typecheck:typecheck@7] [/tmp/steep2/steep_workers/Steepfile] [target=app]   # D = Steep::Diagnostic                          # Define a constant to shorten namespace
..................................................................................

No type error detected. 🫖

@soutaro
Copy link
Owner

soutaro commented Nov 27, 2023

@ParadoxV5 Adding a behavior without Steepfile would make sense.

I'm not sure if type checking everything from the current directory is great or not. It will report many unexpected type errors, from spec, test, or vendor/bundle...

Integration Test:

You can test with cli_test.rb.

end
else
if warnings # Silence warnings in children worker processes
warn "Cannot find a configuration at #{path}: `steep init` to scaffold. Using current directory..."
Copy link
Owner

Choose a reason for hiding this comment

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

Steep.logger.error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never realized Steep uses a logger system. I will check it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be an error or a warning? This PR sought to support non-mandatory Steepfile after all.

Copy link
Owner

Choose a reason for hiding this comment

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

I was afraid if warn is hidden by default, but confirmed the default log level is info.
So, Steep.logger.warn would be the best one.

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’ve replaced it with Steep.logger.warn and written tests but haven’t pushed the changes, for I discovered that Steep.logger.warn doesn’t print whereas both Steep.logger.error and plain warn are fine. Let me know if this is intended and which direction I should move forward.

Copy link
Owner

Choose a reason for hiding this comment

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

@ParadoxV5 I'm sorry. It's my bad. ERROR is the default value and it should be Steep.logger.error. 🙇‍♂️

https://github.com/soutaro/steep/blob/master/lib/steep.rb#L158

Copy link
Contributor Author

@ParadoxV5 ParadoxV5 Dec 4, 2023

Choose a reason for hiding this comment

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

GitHub blame showed that ’twas changed from WARN to ERROR with #322.

Why though? WARN also sounds quite critical. Whereas INFO – the level below WARN – sounds safe to ignore for default configs.

@ParadoxV5
Copy link
Contributor Author

I'm not sure if type checking everything from the current directory is great or not. It will report many unexpected type errors, from spec, test, or vendor/bundle...

I was thinking that users would set up a Steepfile if they have a project structure, similar to Gemfile; but sure, they may very well forget to if they’re not reminded about one.

You can test with cli_test.rb.

👍

@soutaro
Copy link
Owner

soutaro commented Dec 1, 2023

@ParadoxV5 Ok, I'm not completely sure for the feature, but we can implement this. 👍

@ParadoxV5
Copy link
Contributor Author

@ParadoxV5 Ok, I'm not completely sure for the feature, but we can implement this. 👍

What are the aspects that you aren’t confident yet?


I gave steep-vscode a try. VSCode attempts to activate it even when there’s no Gemfile and Steepfile which this LSP depends on. Worse, VSCode retries multiple times and all failures are reported (read: spammed) to the user. This motivated me to write this PR and file soutaro/steep-vscode#296.

@soutaro soutaro added this to the Steep 1.7 milestone Dec 27, 2023
@soutaro soutaro merged commit b5e9ea3 into soutaro:master Dec 27, 2023
15 checks passed
@ParadoxV5 ParadoxV5 deleted the no-Steepfile branch December 28, 2023 05:02
@soutaro soutaro added Released The PR is already included in a published release and removed Released The PR is already included in a published release labels Jan 30, 2024
@soutaro soutaro added the Released The PR is already included in a published release label Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released The PR is already included in a published release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants