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

Handle settings.gradle.kts or missing settings.gradle.dcl #4

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

h0tk3y
Copy link
Member

@h0tk3y h0tk3y commented Nov 11, 2024

No description provided.

@h0tk3y h0tk3y requested a review from hegyibalint November 11, 2024 14:53
@h0tk3y h0tk3y self-assigned this Nov 11, 2024
@h0tk3y h0tk3y force-pushed the sigushkin/settings-gradle-kts-or-none branch from 00a3bf7 to 984be52 Compare November 11, 2024 14:55
Comment on lines 107 to 113
List<File> candidateFiles = Stream.of("settings.gradle.dcl", "settings.gradle.kts")
.map(it -> new File(getRootDir(), "settings.gradle.dcl"))
.collect(Collectors.toList());
return candidateFiles.stream()
.filter(File::exists)
.findFirst()
.orElse(candidateFiles.get(0));
Copy link
Member

Choose a reason for hiding this comment

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

❌ Why collect it meanwhile you are streaming it again? I think this works, and it's much more readable.

Suggested change
List<File> candidateFiles = Stream.of("settings.gradle.dcl", "settings.gradle.kts")
.map(it -> new File(getRootDir(), "settings.gradle.dcl"))
.collect(Collectors.toList());
return candidateFiles.stream()
.filter(File::exists)
.findFirst()
.orElse(candidateFiles.get(0));
List<File> candidateFiles = Stream.of("settings.gradle.dcl", "settings.gradle.kts")
.map(it -> new File(getRootDir(), "settings.gradle.dcl"))
.filter(File::exists)
.findFirst()
.orElse(candidateFiles.get(0));

Copy link
Member Author

Choose a reason for hiding this comment

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

.orElse(candidateFiles.get(0)) is a self-reference in the initializer expression of candidateFiles, or am I not getting it right?

I wanted to avoid duplicating the call to new File(...) and avoid duplicating the name. To avoid both, I had to introduce a named variable to reference later, and that cannot be a stream, as you cannot iterate a stream twice. I'll see how to refactor it to make it more readable.

Copy link
Member Author

@h0tk3y h0tk3y Nov 12, 2024

Choose a reason for hiding this comment

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

Do you think this would be a better option?

            List<String> candidateFileNames = Arrays.asList("settings.gradle.dcl", "settings.gradle.kts");
            Function<String, File> asFileInRootDirectory = it -> new File(getRootDir(), it);
            
            return  candidateFileNames.stream()
                    .map(asFileInRootDirectory)
                    .filter(File::exists)
                    .findFirst()
                    .orElse(asFileInRootDirectory.apply(candidateFileNames.get(0)));

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 this is more readable

@@ -300,7 +300,8 @@ class DeclarativeTextDocumentService : TextDocumentService {
val fileName = uri.path.substringAfterLast('/')
val fileSchema = schemaAnalysisEvaluator.evaluate(fileName, text)
val settingsSchema = schemaAnalysisEvaluator.evaluate(
declarativeResources.settingsFile.name, declarativeResources.settingsFile.readText()
declarativeResources.settingsFile.name,
declarativeResources.settingsFile.takeIf { it.canRead() }?.readText().orEmpty()
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Now I realize we don't handle these cases when adding a new settings file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean that the editor will not react to the creation of a settings file?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - didCreate and didOpen would fire, but we don't have any logic saying: "ah, this is a new root build/setting file for this build"

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to merge this as-is, so we have a (partially, maybe) working build sooner. Let's maybe track the problem you found as an issue?

@h0tk3y h0tk3y merged commit 1a2128b into main Nov 13, 2024
5 checks passed
@Seido30
Copy link

Seido30 commented Nov 22, 2024

Please somebody should help me I have been trying to download Gradle from Gradle site I could not download it, please I need Gradle bin file,the downloading site could not be reachable

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.

3 participants