-
Notifications
You must be signed in to change notification settings - Fork 28
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
Todotnet package #30
Todotnet package #30
Conversation
foreach ($project in $packagingProjects) { | ||
New-NuGetPackageFromProjectFile $project $version | ||
} | ||
|
||
$nuspecProjects = @() | ||
$nuspecProjects = "Json.Schema.ToDotNet.Cli", "Json.Schema.Validation.Cli" |
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.
Cli [](start = 44, length = 3)
I understand why you might want to name the command line tool executables this way. But I don't agree with naming the packages this way. The name is restrictive: it suggests to me that the package contains only the command line tool. Whereas if you named the packages Json.Schema.ToDotNet
, Json.Schema.Validation
, then I would not be surprised to find they contained a command line tool in addition to the library #ByDesign
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.
Oh, I'm sorry! I now see from the .nuspec
file that the package is named as I suggest. It's just the project file that has this name. This is fine.
In reply to: 213830491 [](ancestors = 213830491)
# Microsoft Json Schema Packages | ||
|
||
## **0.57.0** [Pointer](https://www.nuget.org/packages/Microsoft.Json.Pointer/0.57.0) | [Schema](https://www.nuget.org/packages/Microsoft.Json.Schema/0.57.0)| [Schema.ToDotNet](https://www.nuget.org/packages/Microsoft.Json.Schema.ToDotNet/0.57.0)| [Schema.Validation](https://www.nuget.org/packages/Microsoft.Json.Schema.Validation/0.57.0) | ||
* Remove dependency on result.ruleMessageId |
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.
Remove dependency on result.ruleMessageId [](start = 2, length = 41)
How about "The SARIF log file produced by the validation tool no longer uses the obsolete SARIF property result.ruleMessageId
. #Closed
|
||
goto :Exit | ||
|
||
:CopyFilesForMultitargeting | ||
:CopyDllForMultitargeting | ||
xcopy /Y %BinaryOutputDirectory%\%~n1\netstandard2.0\Microsoft.%1 %SigningDirectory%\netcoreapp2.0\ | ||
if "%ERRORLEVEL%" NEQ "0" (echo %1 assembly copy failed. && goto :CopyFilesExit) |
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.
CopyFilesExit [](start = 66, length = 13)
This label no longer exists. Did you mean to move it down to just above Line 54, rather than just removing it? #Closed
|
||
goto :Exit | ||
|
||
:CopyFilesForMultitargeting | ||
:CopyDllForMultitargeting | ||
|
||
xcopy /Y %SigningDirectory%\netcoreapp2.0\Microsoft.%1 %BinaryOutputDirectory%\%~n1\netstandard2.0\ | ||
if "%ERRORLEVEL%" NEQ "0" (echo %1 assembly copy failed. && goto :CopyFilesExit) |
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.
CopyFilesExit [](start = 66, length = 13)
As in CreateSigningDirectory.cmd
, this label no longer exists. #Closed
call :CopyFilesForMultitargeting Json.Pointer.dll || goto :ExitFailed | ||
call :CopyFilesForMultitargeting Json.Schema.ToDotNet.dll || goto :ExitFailed | ||
call :CopyFilesForMultitargeting Json.Schema.Validation.dll || goto :ExitFailed | ||
call :CopyDllForMultitargeting Json.Schema.dll || goto :ExitFailed |
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.
CopyDllForMultitargeting [](start = 6, length = 24)
I've never liked this "ForMultitargeting" part of the name; it took me a while to figure out what it meant. How about CopySignedLibraryToBuildOutputDirectory
. (Note I say "Library" rather than "Dll" because the "Exe" can also be a .dll
; see my other naming suggestion below. #Closed
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.
To clarify: I still don't know what that portion of the name means, but now at least I know what the code does.
In reply to: 213832995 [](ancestors = 213832995)
call :CopyDllForMultitargeting Json.Schema.ToDotNet.dll || goto :ExitFailed | ||
call :CopyDllForMultitargeting Json.Schema.Validation.dll || goto :ExitFailed | ||
|
||
call :CopyExeForMultitargeting Json.Schema.ToDotNet.Cli || goto :ExitFailed |
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.
CopyExeForMultitargeting [](start = 6, length = 24)
Consider: CopySignedExecutableToBuildOutputDirectory
. "Executable" rather than "Exe" because the latter suggests a filename extension, and the file being copied isn't always a .exe
. #Closed
call :CopyFilesForMultitargeting Json.Pointer.dll || goto :ExitFailed | ||
call :CopyFilesForMultitargeting Json.Schema.ToDotNet.dll || goto :ExitFailed | ||
call :CopyFilesForMultitargeting Json.Schema.Validation.dll || goto :ExitFailed | ||
call :CopyDllForMultitargeting Json.Schema.dll || goto :ExitFailed |
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.
CopyDllForMultitargeting [](start = 6, length = 24)
Consider: CopyUnsignedLibraryToSigningDirectory
. See the reasoning in my comments on PushSignedBinariesIntoBuild.cmd
. #Closed
call :CopyDllForMultitargeting Json.Schema.ToDotNet.dll || goto :ExitFailed | ||
call :CopyDllForMultitargeting Json.Schema.Validation.dll || goto :ExitFailed | ||
|
||
call :CopyExeForMultitargeting Json.Schema.ToDotNet.Cli || goto :ExitFailed |
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.
CopyExeForMultitargeting [](start = 6, length = 24)
Consider: CopyUnsignedExecutableToSigningDirectory
. Again, see my comments on PushSignedBinariesInfoBuild.cmd
. #Closed
@@ -3,8 +3,9 @@ | |||
<PropertyGroup> | |||
<OutputType>Exe</OutputType> | |||
<TargetFrameworks>netcoreapp2.0</TargetFrameworks> | |||
<AssemblyNameOverride>JsonSchemaToDotNet</AssemblyNameOverride> | |||
<AssemblyNameOverride>Microsoft.Json.Schema.ToDotNet.Cli</AssemblyNameOverride> | |||
<RootNamespaceOverride>Microsoft.Json.Schema.ToDotNet.CommandLine</RootNamespaceOverride> |
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.
RootNamespaceOverride [](start = 5, length = 21)
Now that the assembly has a name that matches the project name, we don't need these overrides at all. We can use the defaults defined in build.props
. We'd just need to change the namespaces in the two source files in this directory. #Closed
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.
@michaelcfanning Could you explain why? It's a quick fix and, in fact, with the fix in place, I can even remove the "override" logic in build.props
(it's only there because for the tools, the exe name didn't match the project name).
Would you like me to take care of it in a separate PR?
In reply to: 213834909 [](ancestors = 213834909)
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.
<metadata> | ||
<id>Microsoft.Json.Schema.Validation</id> | ||
<version>$version$</version> | ||
<title>Microsoft Json Schema Validation</title> |
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.
Json [](start = 21, length = 4)
"JSON" here and throughout user-facing text. ("Json" is for programmatic elements in .NET code.) #Closed
<owners>microsoft,tse-securitytools</owners> | ||
<requireLicenseAcceptance>false</requireLicenseAcceptance> | ||
<description>A utility that generates a .NET object model from a JSON schema.</description> | ||
<releaseNotes>Version $version$ of the JSON .NET object model generator (for SARIF v2.0.0)</releaseNotes> |
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.
for SARIF v2.0.0 [](start = 77, length = 16)
There is nothing SARIF v2.0-specific about this tool. It will generate an object model for any schema (with certain limitations on the JSON schema constructs it can handle). It just happens that in building the SARIF SDK, we run it against the SARIF v2.0 schema. #Closed
<owners>microsoft,tse-securitytools</owners> | ||
<requireLicenseAcceptance>false</requireLicenseAcceptance> | ||
<description>A utility that validates files according to a JSON schema.</description> | ||
<releaseNotes>Version $version$ of the JSON validation utility (for SARIF v2.0.0)</releaseNotes> |
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.
for SARIF v2.0.0 [](start = 68, length = 16)
There is slightly more justification for this phrase here than there was in the ToDotNet .nuspec file, but I would still omit it. This tool emits its output in SARIF v2.0. Is that what you meant by this phrase? #Closed
* Remove dependency on RuleMessageId (shortly to be deprecated) (#24) * Release 0.57.0 * Todotnet package (#30) * Add utilities to validation and todotnet packages * Update nuget.exe. Add success message for nuspec-driven package creation. * Update signing scripts * Release 0.58.0 (#32) * Address PR feedback from 0.58.0 release (#33) Also: * Build the `develop` branch in AppVeyor. * Add dotnet publish step to build. (#34) Also: - Use `.CommandLine`, not `.Cli` in the command line tool namespaces. * Disable appveyor email notifications (#36) * Update to latest SARIF SDK + JSON.NET downgrade (#38) * Fix ToDotNet.Cli project file so resources can be found. (#40) When you run the ToDotNet command line app, it fails because it can't find the resource stream. Somehow the `.resx` and the `.Resources.designer` files got separated in the project file. Fix the project file to connect the resources back up. * Rewrite top-level README in preparation for documenting each component. (#47) An almost complete rewrite. * Fix #41: Document Json.Pointer (#48) * embed resx resources (#53) * Add unit test to demonstrate interaction of ClassNameHint with other hints (#54) * Start codegen README (#59) * Fix #57, fix #58: Emit initializers and attributes for default values (#60) #57: When a schema property declares a default value, assign that value to the property in the default constructor. This ensures that the property has the correct default even if you construct the object by hand rather than by deserializing it from JSON. #58: When a schema property declares a default value, decorate the property with a `System.ComponentModel.DefaultValue` attribute. This avoids your having to specify those attributes in the CodeGenHints.json file, which is error-prone. Also: - Fix a bug where the generated doc comments for properties had an extra space: `cref="P: propName`" → `cref="P:propName`". - Address some hygiene-related IDE messages by marking some properties `readonly`, and by inlining the declarations of some `out` parameters. * Fix #61: Generate JsonProperty attribute for properties with defaults (#62) * Add final release note in advance of shipping 0.59.0
* Remove dependency on RuleMessageId (shortly to be deprecated) (#24) * Release 0.57.0 * Todotnet package (#30) * Add utilities to validation and todotnet packages * Update nuget.exe. Add success message for nuspec-driven package creation. * Update signing scripts * Release 0.58.0 (#32) * Address PR feedback from 0.58.0 release (#33) Also: * Build the `develop` branch in AppVeyor. * Add dotnet publish step to build. (#34) Also: - Use `.CommandLine`, not `.Cli` in the command line tool namespaces. * Disable appveyor email notifications (#36) * Update to latest SARIF SDK + JSON.NET downgrade (#38) * Fix ToDotNet.Cli project file so resources can be found. (#40) When you run the ToDotNet command line app, it fails because it can't find the resource stream. Somehow the `.resx` and the `.Resources.designer` files got separated in the project file. Fix the project file to connect the resources back up. * Rewrite top-level README in preparation for documenting each component. (#47) An almost complete rewrite. * Fix #41: Document Json.Pointer (#48) * embed resx resources (#53) * Add unit test to demonstrate interaction of ClassNameHint with other hints (#54) * Start codegen README (#59) * Fix #57, fix #58: Emit initializers and attributes for default values (#60) #57: When a schema property declares a default value, assign that value to the property in the default constructor. This ensures that the property has the correct default even if you construct the object by hand rather than by deserializing it from JSON. #58: When a schema property declares a default value, decorate the property with a `System.ComponentModel.DefaultValue` attribute. This avoids your having to specify those attributes in the CodeGenHints.json file, which is error-prone. Also: - Fix a bug where the generated doc comments for properties had an extra space: `cref="P: propName`" → `cref="P:propName`". - Address some hygiene-related IDE messages by marking some properties `readonly`, and by inlining the declarations of some `out` parameters. * Fix #61: Generate JsonProperty attribute for properties with defaults (#62) * Add final release note in advance of shipping 0.59.0 * Update licensing reference to satisfy NuGet improvements in this area. (#65)
Rename OM generator and validation to match dll names + 'Cli.exe'. Inject these utilities into the core assembly package. Update signing scripts. Also updated nuget.exe to current version that has updated valid framework target names.
@lgolding @Evmaus-MS