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.

Sign up for GitHub

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

Feature/3465 support for java variants #3797

Merged
merged 10 commits into from
Aug 2, 2022
Merged

Feature/3465 support for java variants #3797

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a0b0dcf
added Variant field to reviews and codefiles for different language v…
Vailorium Aug 1, 2022
f75dd2c
added basic java variant detection based on package name
Vailorium Aug 1, 2022
00d9911
add java variant icon next to language icon
Vailorium Aug 1, 2022
63ee9f1
replaced java variant string with enum
Vailorium Aug 1, 2022
0c4c810
Replace language icon with variant if variant exists
Vailorium Aug 1, 2022
bb181bf
Capitalize java LanguageVariant enum values, added variantName value
Vailorium Aug 1, 2022
87122ed
fix conditional placement for java variant icons
Vailorium Aug 1, 2022
ec58043
solve language variant merge conflict
Vailorium Aug 1, 2022
bb95588
add variant to review page alt text
Vailorium Aug 1, 2022
941a8de
change Variant to LanguageVariant, base java variant on groupId inste…
Vailorium Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/dotnet/APIView/APIView/Model/CodeFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public string VersionString

public string Language { get; set; }

public string Variant { get; set; }

public string PackageName { get; set; }

public string ServiceName { get; set; }
Expand Down
5 changes: 3 additions & 2 deletions src/dotnet/APIView/APIViewWeb/Models/ReviewCodeFileModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ public class ReviewCodeFileModel
// This is field is more of a display name. It is set to name value returned by parser which has package name and version in following format
// Package name ( Version )
public string Name { get; set; }

public string Language
{
get => _language ?? (Name.EndsWith(".json", StringComparison.OrdinalIgnoreCase) ? "Json" : "C#");
Expand All @@ -23,6 +22,8 @@ public string Language

public string VersionString { get; set; }

public string Variant { get; set; }

public bool HasOriginal { get; set; }

public DateTime CreationDate { get; set; } = DateTime.Now;
Expand All @@ -36,4 +37,4 @@ public string Language
// This field stores original file name uploaded to create review
public string FileName { get; set; }
}
}
}
3 changes: 3 additions & 0 deletions src/dotnet/APIView/APIViewWeb/Models/ReviewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ public string DisplayName
[JsonIgnore]
public string Language => Revisions.LastOrDefault()?.Files.LastOrDefault()?.Language;

[JsonIgnore]
public string Variant => Revisions.LastOrDefault()?.Files.LastOrDefault()?.Variant;

[JsonIgnore]
public string PackageName {
get
Expand Down
19 changes: 18 additions & 1 deletion src/dotnet/APIView/APIViewWeb/Pages/Assemblies/Review.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,24 @@
<span><img class="mx-1" src="~/icons/go-original.svg" width="40" alt="@Model.Review.Language"></span>
break;
case "java":
<span><img class="mx-1" src="~/icons/java-original.svg" width="40" alt="@Model.Review.Language"></span>
if(@Model.Review.Variant != null)
{
switch(@Model.Review.Variant.ToLower())
{
case "spring":
<span><img class="mx-1" src="~/icons/java-spring-original.svg" width="40" alt="@Model.Review.Language @Model.Review.Variant"></span>
break;
case "android":
<span><img class="mx-1" src="~/icons/java-android-original.svg" width="40" alt="@Model.Review.Language @Model.Review.Variant"></span>
break;
default:
<span><img class="mx-1" src="~/icons/java-original.svg" width="40" alt="@Model.Review.Language"></span>
break;
Copy link
Member

@JonathanGiles JonathanGiles Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Eventually I would like to see this change to instead be a single line that uses code along the following lines:

if (variant != null && !variant.isEmpty()) {
    // set the img class property to be 'language-variant' (e.g. 'java-android')
} else {
    // set the img class property to be 'language' (e.g. 'java')
}

Then move all of the image file references out to CSS, e.g.

img.java {

}
img.java-android {

}

}
} else
Copy link
Member

Choose a reason for hiding this comment

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

Same here

{
<span><img class="mx-1" src="~/icons/java-original.svg" width="40" alt="@Model.Review.Language"></span>
}
break;
case "swift":
<span><img class="mx-1" src="~/icons/swift-original.svg" width="40" alt="@Model.Review.Language"></span>
Expand Down
19 changes: 18 additions & 1 deletion src/dotnet/APIView/APIViewWeb/Pages/Shared/_ReviewsPartial.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,24 @@
<img class="mx-1" src="~/icons/go-original.svg" alt="@review.Language">
break;
case "java":
<img class="mx-1" src="~/icons/java-original.svg" alt="@review.Language">
if(@review.Variant != null)
{
switch(@review.Variant.ToLower())
{
case "spring":
<img class="mx-1" src="~/icons/java-spring-original.svg" alt="@review.Language @review.Variant">
break;
case "android":
<img class="mx-1" src="~/icons/java-android-original.svg" alt="@review.Language @review.Variant">
break;
default:
<img class="mx-1" src="~/icons/java-original.svg" alt="@review.Language">
break;
}
} else
Copy link
Member

Choose a reason for hiding this comment

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

As much as it pains me (as a Java engineer), the C# expectation here is to put the closing brace and the else on separate lines.

{
<img class="mx-1" src="~/icons/java-original.svg" alt="@review.Language">
}
break;
case "swift":
<img class="mx-1" src="~/icons/swift-original.svg" alt="@review.Language">
Expand Down
1 change: 1 addition & 0 deletions src/dotnet/APIView/APIViewWeb/Repositories/ReviewManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ public async Task ToggleIsClosedAsync(ClaimsPrincipal user, string id)
private void InitializeFromCodeFile(ReviewCodeFileModel file, CodeFile codeFile)
{
file.Language = codeFile.Language;
file.Variant = codeFile.Variant;
file.VersionString = codeFile.VersionString;
file.Name = codeFile.Name;
file.PackageName = codeFile.PackageName;
Expand Down
54 changes: 54 additions & 0 deletions src/dotnet/APIView/APIViewWeb/wwwroot/icons/java-android-original.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions src/dotnet/APIView/APIViewWeb/wwwroot/icons/java-spring-original.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
10 changes: 10 additions & 0 deletions src/java/apiview-java-processor/src/main/java/com/azure/tools/apiview/processor/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.azure.tools.apiview.processor.model.APIListing;
import com.azure.tools.apiview.processor.model.Diagnostic;
import com.azure.tools.apiview.processor.model.DiagnosticKind;
import com.azure.tools.apiview.processor.model.LanguageVariant;
import com.azure.tools.apiview.processor.model.Token;
import com.azure.tools.apiview.processor.model.maven.Pom;
import com.fasterxml.jackson.annotation.JsonInclude;
Expand Down Expand Up @@ -157,6 +158,15 @@ private static void processJavaSourcesJar(File inputFile, APIListing apiListing)
apiListing.setLanguage("Java");
apiListing.setMavenPom(reviewProperties.getMavenPom());

if(packageName.contains("spring")) {
Copy link
Member

@srnagar srnagar Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: let's use groupId.contains("spring") instead of packageName(which is groupId:artifactId) to ensure artifact names that happen to contain spring (not very likely) aren't classified as Spring libraries. As @JonathanGiles suggested below, we may want a different way to identify this but until then, we can go by the group name.

Just to add a bit more context here, we use groupId to differentiate between Spring, Android and Java Azure libraries.

com.azure - Java libraries
com.azure.android - Android libraries
com.azure.spring - Spring libraries.

Artifact Ids can be any of the hundreds of Azure service names and there's a chance that they may contain terms like "spring" and are not related to the Spring framework.

apiListing.setVariant(LanguageVariant.SPRING);
} else if(packageName.contains("android")) {
apiListing.setVariant(LanguageVariant.ANDROID);
} else {
apiListing.setVariant(LanguageVariant.DEFAULT);
}
System.out.println(" Using '" + apiListing.getVariant() + "' for the language variant");
Copy link
Member

Choose a reason for hiding this comment

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

Eventually we will want a smarter way of discerning this, but it should do for now


final Analyser analyser = new JavaASTAnalyser(inputFile, apiListing);

// Read all files within the jar file so that we can create a list of files to analyse
Expand Down
11 changes: 11 additions & 0 deletions ...view-java-processor/src/main/java/com/azure/tools/apiview/processor/model/APIListing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ public class APIListing {
@JsonProperty("Language")
private String language;

@JsonProperty("Variant")
Copy link
Member

Choose a reason for hiding this comment

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

nit: Let's name the property LanguageVariant to match the type and is more specific than just variant. You might want to consider doing that in other places too.

private LanguageVariant variant;

@JsonProperty("PackageName")
private String packageName;

Expand Down Expand Up @@ -91,6 +94,14 @@ public void setLanguage(final String language) {
this.language = language;
}

public LanguageVariant getVariant() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: getLanguageVariant()

return variant;
}

public void setVariant(final LanguageVariant variant) {
this.variant = variant;
}

public String getPackageName() {
return packageName;
}
Expand Down
21 changes: 21 additions & 0 deletions ...java-processor/src/main/java/com/azure/tools/apiview/processor/model/LanguageVariant.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package com.azure.tools.apiview.processor.model;

import com.fasterxml.jackson.annotation.JsonValue;

public enum LanguageVariant {
DEFAULT("Default"),
SPRING("Spring"),
ANDROID("Android");

private final String variantName;

LanguageVariant(String name) {
this.variantName = name;
}

@Override
@JsonValue
public String toString() {
return this.variantName;
}
}