-
Notifications
You must be signed in to change notification settings - Fork 8
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
support validation_packages_yml
(continued)
#409
Conversation
validation_packages_yml (continued)
validation_packages_yml
(continued)
@Freymaurer finally done adding all those tests 🥇 |
3905a62
to
ba8d69e
Compare
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.
Awesome tests ✅ Requested some minor changes!
@@ -22,6 +22,15 @@ type ValidationPackage(name, ?version) = | |||
member this.Copy() = | |||
ValidationPackage.make this.Name this.Version | |||
|
|||
/// Pretty printer | |||
override this.ToString() = |
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.
Maybe do not use yaml style for pretty printing, or use YAMLicious to generate this. But i would prefer typical f# printing syntax
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.
If this is just taste, i would prefer keeping it as many tests would need to be adapted
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.
Also, i think the to string is valuable here because it represents something that only ever exists as a yaml file in the arc.
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.
It is just that the ToString function should not be responsible for YAML formatting, this should be a different function.
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.
it is not. it is just for printing. formatting for actual yaml export is done with yamlicious.
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.
Then why do we have tests for this? 😮
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.
i just tested everything that i added
| :? ValidationPackagesConfig as other_vpc -> other_vpc.ValidationPackages = this.ValidationPackages && other_vpc.ARCSpecification = this.ARCSpecification | ||
|
||
/// Pretty printer | ||
override this.ToString() = |
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.
Same as ValidationPackage.ToString()
d0ae4d0
to
8de0653
Compare
…config file, add related tests
This PR continues #403 :