Skip to content

Add permissive and strict library.properties JSON schemas #25

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

Merged
merged 6 commits into from
Nov 19, 2020

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Nov 17, 2020

I started the arduino-check project with the simplistic vision of it being a tool used to enforce the Arduino Sketch/Library/Platform/Package Index Specifications. However this approach neglects some important use cases:

  • Library Manager index management. Specification violations don't always result in breakage, so library authors may resent a strict requirement that all libraries added to the index be specification-compliant. More significantly, large numbers of non-compliant libraries are already in the index, so a change to blocking non-specification compliant releases would likely be very disruptive. For this reason, a "permissive" subset of the specification is needed.

  • Advocating best practices. Although not mandated by the specification, some practices result in an improved experience for the user, and thus should be advocated by Arduino. It would be unreasonable to enforce these practices on unwilling users, but they can be advocated for with warnings and by offering willing users an option to use the tool in a "pedantic" mode. For this reason, a "strict" superset of the specification is needed.

JSON schemas offer a standardized method of defining a data format. This form of information can be easily used by consumers other than the arduino-check tool. So I think it's the best approach to define these compliance levels in schemas, rather than directly in Go code.

Significant components of the schemas are identical. To avoid the need to maintain duplicate content, it's important to use referencing. While working on this, I found that it it was confusing to work with a separate file for the definition of each compliance level of each document property, especially when considering how changing one schema affects all the schemas that reference it, and the schemas that reference those schemas... I settled on the approach of using a monolithic definitions schema, resulting in the parent schema being nearly identical collection of references to that file, which are unlikely to need to be updated at all in the future, even when modifications are made to the definitions schema.

With the idea that schemas for the other Arduino project configuration files will be created in the future to expand the scope of arduino-check, I put the definitions that can be used by other schemas in a dedicated definitions schema, from which they can be referenced by additional schemas.

@per1234 per1234 requested a review from silvanocerza November 17, 2020 07:57
@per1234 per1234 force-pushed the per1234/library_properties-schema branch from 5edeafe to bed28de Compare November 17, 2020 08:02
I started the arduino-check project with the simplistic vision of it being a tool used to enforce the Arduino
Sketch/Library/Platform/Package Index Specifications. However this approach neglects some important use cases:

- Library Manager index management. Specification violations don't always result in breakage, so library authors may
resent a strict requirement that all libraries added to the index be specification-compliant. More significantly, large
numbers of non-compliant libraries are already in the index, so a change to blocking non-specification compliant
releases would likely be very disruptive. For this reason, a "permissive" subset of the specification is needed.

- Advocating best practices. Although not mandated by the specification, some practices result in an improved experience
for the user, and thus should be advocated by Arduino. It would be unreasonable to enforce these practices on unwilling
users, but they can be advocated for with warnings and by offering willing users an option to use the tool in a
"pedantic" mode. For this reason, a "strict" superset of the specification is needed.

JSON schemas offer a standardized method of defining a data format. This form of information can be easily used by
consumers other than the arduino-check tool. So I think it's the best approach to define these compliance levels in
schemas, rather than directly in Go code.

Significant components of the schemas are identical. To avoid the need to maintain duplicate content, it's important to
use referencing. While working on this, I found that it it was confusing to work with a separate file for the definition
of each compliance level of each document property, especially when considering how changing one schema affects all the
schemas that reference it, and the schemas that reference those schemas... I settled on the approach of using a
monolithic definitions schema, resulting in the parent schema being nearly identical collection of references to that
file, which are unlikely to need to be updated at all in the future, even when modifications are made to the
definitions schema.

With the idea that schemas for the other Arduino project configuration files will be created in the future to expand the
scope of arduino-check, I put the definitions that can be used by other schemas in a dedicated definitions schema, from
which they can be referenced by additional schemas.
@per1234 per1234 force-pushed the per1234/library_properties-schema branch from bed28de to 2d758b0 Compare November 17, 2020 08:13
@per1234 per1234 requested a review from rsora November 17, 2020 09:05
// To purchase a commercial license, send an email to license@arduino.cc.

// Package schemas contains tests for the JSON schemas.
package schemas
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'm not sure whether I took the right approach by creating this empty package in order to have the JSON schemas tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

What are you actually testing here? Looking at the tests I'd probably put them under the libraryproperties package, but I might be wrong.
I think it's kinda strange creating an empty package for testing by the way. Have you tried deleting this file and leave only its related _test.go? Not sure if it works really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are you actually testing here?

The configuration of the JSON schemas themselves, the tests in etc/schemas/test/librarypropertiesschemas_test.go are not targeted at testing the Go code.

I'd probably put them under the libraryproperties package

OK, if you think that's the better place, I'll move them there: 3a0bf88

Have you tried deleting this file and leave only its related _test.go?

That's what I did originally, but it causes task go:test-unit to fail:
https://github.com/per1234/arduino-check/runs/1414468912?check_suite_focus=true#step:6:12

go build github.com/arduino/arduino-check/etc/schemas/test: no non-test Go files in /home/runner/work/arduino-check/arduino-check/etc/schemas/test
?   	github.com/arduino/arduino-check	[no test files]
?   	github.com/arduino/arduino-check/check	[no test files]
?   	github.com/arduino/arduino-check/check/checkconfigurations	[no test files]
?   	github.com/arduino/arduino-check/check/checkdata	[no test files]
FAIL	github.com/arduino/arduino-check/check/checkdata/schema [build failed]

I did some research and found the fix of adding an empty go file:
https://github.com/golang/go/issues/22409#issuecomment-382985756

This can be solved with a simple workaround of adding any dummy file with a non _test suffix in the same package. Just have a ignore.go file with a one-liner package tests, and everything works.

https://github.com/golang/go/issues/8279#issuecomment-66096402

Adding a dummy.go file (with only "package " line) works

https://stackoverflow.com/a/47025370/7059512 / https://stackoverflow.com/a/62394096/7059512

Add doc.go file to your tests pkg and call it package tests in both files

@per1234 per1234 merged commit 47f9cd1 into main Nov 19, 2020
@per1234 per1234 deleted the per1234/library_properties-schema branch November 19, 2020 11:13
@per1234 per1234 added topic: code Related to content of the project itself type: enhancement Proposed improvement labels Sep 29, 2021
@per1234 per1234 self-assigned this Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants