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

Linter: check the license_family is valid #339

Merged
merged 3 commits into from
Nov 9, 2016

Conversation

kynan
Copy link
Contributor

@kynan kynan commented Oct 28, 2016

conda-build actually checks for that. Relates to #88.

@@ -86,7 +88,13 @@ def lintify(meta, recipe_dir=None):
if 'unknown' == license.strip():
lints.append('The recipe license cannot be unknown.')

# 6: Selectors should be in a tidy form.
# 6: License family must be valid (conda-build checks for that)
Copy link
Member

@jakirkham jakirkham Oct 28, 2016

Choose a reason for hiding this comment

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

I don't know that we want to be messing with the ordering. Maybe @pelson can comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to keep the existing numbering, do you prefer adding the check to the end? Or making it 6b or such?

Copy link
Member

Choose a reason for hiding this comment

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

Let's go ahead and put it at the end. May need to rebase though as I just merged PR ( #334 ).

@jakirkham
Copy link
Member

It's a bit redundant as conda-build does check it. That being said, am in favor of not having to hunt through logs for information like this. So having the linter say it is an improvement.

@kynan
Copy link
Contributor Author

kynan commented Oct 29, 2016

I agree it's a redundant check, but it will be detected at lint rather than build time.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Once the number issue is addressed would be happy to merge.

@kynan kynan force-pushed the lint-license-family branch from 377d920 to fb5c3f6 Compare November 6, 2016 00:05
@kynan
Copy link
Contributor Author

kynan commented Nov 6, 2016

@jakirkham done!

license_family = about_section.get('license_family', '')
if license_family and not license_family in allowed_license_families:
lints.append('The recipe license_family `{}` is invalid: must be one of {}.'
''.format(license_family, ', '.join(allowed_license_families)))
Copy link
Member

Choose a reason for hiding this comment

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

Actually why don't we just use ensure_valid_license_family to verify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do. I assume we want to catch the exception and add the message to lints?

Copy link
Member

Choose a reason for hiding this comment

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

That's certainly an option and should be easy to do. 👍

@kynan
Copy link
Contributor Author

kynan commented Nov 7, 2016

@jakirkham now using ensure_valid_license_family directly. The build was cancelled, can't tell why.

@jakirkham
Copy link
Member

The build was cancelled, can't tell why.

Weird. 😕 Restarted and seems to have worked.

However, there appears to be a merge conflict. Likely due to another linter change.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

LGTM.

Needs merge conflicts resolved. Then should be good to go.

@kynan kynan force-pushed the lint-license-family branch from 0051878 to 26a3815 Compare November 8, 2016 20:11
@kynan
Copy link
Contributor Author

kynan commented Nov 8, 2016

@jakirkham rebased on upstream master!

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jakirkham jakirkham merged commit bac0d13 into conda-forge:master Nov 9, 2016
@kynan kynan deleted the lint-license-family branch November 9, 2016 13:06
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.

2 participants