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

Add library for deprecated #6146

Merged
merged 2 commits into from
Jun 29, 2018
Merged

Conversation

mariusvniekerk
Copy link
Member

No description provided.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/deprecated) and found it was in an excellent condition.

Copy link
Member

@sodre sodre left a comment

Choose a reason for hiding this comment

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

thanks for adding this, it was on my radar as well 👍

{% set version = "1.2.0" %}
{% set file_ext = "tar.gz" %}
{% set hash_type = "sha256" %}
{% set hash_value = "42b5dffc4023401148e9d2757f46a4a9fcccf114547a4620347ae45f9e77a21c" %}
Copy link
Member

Choose a reason for hiding this comment

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

Should hash_type and hash_value be moved directly under the source section?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably best to just keep close to what conda skeleton spit out since that is one of the major things that the tick bot hits

Copy link
Member

Choose a reason for hiding this comment

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

@CJ-Wright, can you clarify what is the optimal way to write the SHA256 sums? It looks like skeleton and example are conflicting with each other.

Copy link
Member

Choose a reason for hiding this comment

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

For simple recipes we support both I think. The main issue is when we have multiple checksums in a single recipe, then the bot gets confused with the jinja2 method (since it doesn't know which checksum goes to which jinja2)
Lets double check with @justcalamari

Choose a reason for hiding this comment

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

That's correct. If there are multiple sources, the bot doesn't know which checksum the jinja2 variable is for, so it might replace it incorrectly. In this recipe where there is only one checksum, there is no ambiguity so the bot should be able to replace the jinja variable correctly.

It might be good practice to get rid of the jinja variables for hash_type and hash_value though, since there are certain cases where that breaks the bot and it doesn't seem like there's much of a reason to use the variables since they are only used once. I think @ocefpaf changed the example to remove jinja variables for checksum, but skeleton wasn't updated yet.

host:
- python
- pip
- wrapt <2,>=1
Copy link
Member

Choose a reason for hiding this comment

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

I checked upstream's setup.py and it looks like wrapt is only an install requirement. Do we really need it in the host section?

Copy link
Member Author

Choose a reason for hiding this comment

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

let me remove that

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems one of the modules does use it, readding

license: MIT
license_family: MIT
# https://github.com/tantale/deprecated/pull/1
# license_file: ''
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to wait for the upstream PR to merged before merging into conda-forge?

Copy link
Member Author

@mariusvniekerk mariusvniekerk Jun 22, 2018

Choose a reason for hiding this comment

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

I can deal with adding it in post merge probably, but lets give it a day or two

@mariusvniekerk mariusvniekerk force-pushed the deprecated branch 2 times, most recently from b8397db to 81cefa9 Compare June 22, 2018 20:52
@sodre sodre merged commit 5f7e973 into conda-forge:master Jun 29, 2018
@mariusvniekerk mariusvniekerk deleted the deprecated branch December 27, 2018 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants