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

[4.0] Embedded license handling for binding generator #836

Merged
merged 6 commits into from
Nov 1, 2018
Merged

[4.0] Embedded license handling for binding generator #836

merged 6 commits into from
Nov 1, 2018

Conversation

dnstag
Copy link
Contributor

@dnstag dnstag commented Oct 30, 2018

Purpose of this PR

As of #829, the LicenseText property in GeneratorSettingsBase (including parent and child classes) contains actual license data which should be in another place. This PR will implement a class which loads the license file as an embedded resource acessible via property and changes affected classes accordingly.

The PR is marked as WIP as this is my first PR, so please feel free to review and discuss as i commit more changes.

Oh and please... be gentle. :)

Testing status

Sucessfully tested with the text in Specifications/License.txt.

Related

Resolves #829

@Nihlus
Copy link
Contributor

Nihlus commented Oct 30, 2018

Thanks a ton! I'll leaf through it and give you some feedback :) Fair warning, I'm a nitpicky arse :P

src/Generator.Bind/LicenseResource.cs Outdated Show resolved Hide resolved
src/Generator.Bind/LicenseResource.cs Outdated Show resolved Hide resolved
src/Generator.Bind/LicenseResource.cs Outdated Show resolved Hide resolved
src/Generator.Bind/LicenseResource.cs Outdated Show resolved Hide resolved
src/Generator.Bind/LicenseResource.cs Outdated Show resolved Hide resolved
src/Generator.Bind/LicenseResource.cs Outdated Show resolved Hide resolved
src/Generator.Bind/LicenseResource.cs Outdated Show resolved Hide resolved
@Nihlus
Copy link
Contributor

Nihlus commented Oct 30, 2018

Looks like a solid approach - I like it. Some layout issues, spotted a bug, and had a thought about using Lazy instead of manually checking a backing field.

Copy link
Contributor

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

Fix the code where you get the manifest resource stream - I commented on @Nihlus' review. Also, don't forget to test this!

@dnstag
Copy link
Contributor Author

dnstag commented Oct 30, 2018

@DylanFPS Is there a rule for testing internal classes (e.g. location in the source tree, language used) etc.?

@Perksey
Copy link
Contributor

Perksey commented Oct 30, 2018

Just make sure it works, and do it however. And make sure you delete all testing code after you’ve verified it works.

* Renamed LicenseResource to EmbeddedResources.
* Removed Property LicenseText from GeneratorSettings classes.
* Modified BindingWriter to access the license text via embedded resources.
Copy link
Contributor

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

Close!

src/Generator.Bind/EmbeddedResources.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

Fixed the license header myself

@Perksey Perksey requested a review from Nihlus November 1, 2018 15:41
@Nihlus Nihlus merged commit e911f0b into opentk:4.0 Nov 1, 2018
@dnstag dnstag deleted the binding-generator-license branch November 1, 2018 17:09
@varon
Copy link
Member

varon commented Nov 1, 2018

Thanks for the contribution @dnstag - We really hope to see more of this in the future!

@dnstag
Copy link
Contributor Author

dnstag commented Nov 1, 2018

You will! I'm glad i could help. :)

@VPeruS
Copy link
Contributor

VPeruS commented Nov 1, 2018

[WIP] must be removed

@Perksey Perksey changed the title [4.0] [WIP] Embedded license handling for binding generator [4.0] Embedded license handling for binding generator Nov 1, 2018
@varon
Copy link
Member

varon commented Nov 1, 2018

@VPeruS Thanks.

We just added a check for that before merging now.

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.

None yet

5 participants