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

[INFRA] Update Pipfile.lock #144

Merged
merged 2 commits into from
Feb 14, 2019

Conversation

franklin-feingold
Copy link
Collaborator

update pyyaml to 4.2b4

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

I am pretty sure you know what you are doing here ... but I don't really get what the "hashes" do and why we are deleting/replacing some - could you briefly explain, please?

@franklin-feingold
Copy link
Collaborator Author

This is updating our pyyaml package for readthedocs. The hashes are allowing our continuous integrater (CircleCI) to download the required packages (similar to a requirements.txt file, for our case we are using a pipfile.lock) and build our rendering automatically. The hashes ensure package integrity.

I'm sorry for the confusion. Thank you for taking the time to review this!

@sappelhoff
Copy link
Member

okay I see - thanks! Not sure whether we have to wait for some time now until we merge according to our new contributing/merging guidelines ...

@franklin-feingold
Copy link
Collaborator Author

I agree, I think to adhere to the new guidelines we can wait 2 days and see if anyone in the community is interested in further evaluating this PR. If not, I will merge in.

@emdupre
Copy link
Collaborator

emdupre commented Feb 12, 2019

Just to pop in, the changes look very reasonable 👍

I think the way the decision-making is written now, we technically need to wait 5 days to merge. Is that right, @chrisfilo ?

For small changes like this, it might be worth updating the decision making rules so that they have a different waiting period.

@emdupre
Copy link
Collaborator

emdupre commented Feb 12, 2019

Or, is it that the PR has just been open for 5 days, not that it's had approval for 5 ?

@franklin-feingold
Copy link
Collaborator Author

Regarding the 5 days to merge - that is my interpretation, 5 days from the last commit. If a PR was opened and commits were being put into it, then it would have to wait 5 days from the last commit on the PR regardless of how long the PR has been open. I didn't interpret a length from approval to merge, just on the commits.

It could be worth having exceptions (i.e. general github maintenance), then perhaps it could be an admin that is able to provide the exception?

@sappelhoff
Copy link
Member

5 days from the last commit.

Yes, that sounds reasonable because the content is there to review for everybody regardless of whether an approving review is present or not ... and 5 days sounds good: One work-week, where everybody might spare their amount of time they dedicate to BIDS (be it 10 minutes or 60 .. or more).

It could be worth having exceptions (i.e. general github maintenance)

I am not sure that these exceptions are necessary, they would clutter our concise set of rules, wouldn't they? I can't imagine a maintenance PR being so crucial that it needs to be merged ASAP

@chrisgorgo
Copy link
Contributor

5 days from the last commit
This is the correct interpretation. If the wording seems unclear please send a PR to improve it.

As for exceptions - this does not seem like the optimal place to discuss this. Please open a new issue or PR with the suggested change.

@franklin-feingold franklin-feingold merged commit 1aae509 into bids-standard:master Feb 14, 2019
@sappelhoff sappelhoff changed the title Update Pipfile.lock [INFRA] Update Pipfile.lock Sep 8, 2020
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.

4 participants