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.

Sign up for GitHub

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

Jump to bottom

Fix px4.py #14716

Merged
merged 3 commits into from
Apr 21, 2020
Merged

Fix px4.py #14716

merged 3 commits into from
Apr 21, 2020

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented Apr 20, 2020

AFAIK this is only used by the PX4 homebrew formulae, though it makes sense to at least make it usable and correct.

@TSC21 TSC21 requested review from LorenzMeier and julianoes April 20, 2020 13:39
@TSC21 TSC21 changed the title Fix/px4 py Fix px4.py Apr 20, 2020
@dagar
Copy link
Member

dagar commented Apr 20, 2020

If this is for homebrew can we add some basic explanation near the top? Otherwise it looks like the kind of thing someone might accidentally delete.

@TSC21 TSC21 requested a review from dagar April 20, 2020 17:29
@julianoes
Copy link
Contributor

This file is copied in https://github.com/PX4/homebrew-px4/blob/3c71517245ea433416a30262d7abe3acb9bde53e/Formula/px4-dev.rb#L23

This means it is not in the git repo anymore and won't be able to print the version, right?

I don't think this is actually needed. I would either try to remove the copy of this file or replace the printing of the version with just print("PX4 deps installed via homebrew, this is just a dummy script").

julianoes
julianoes requested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

As written in the comment, I don't think it would work as intended.

@LorenzMeier LorenzMeier merged commit c3ebb1a into master Apr 21, 2020
@LorenzMeier LorenzMeier deleted the fix/px4_py branch April 21, 2020 07:21
@julianoes
Copy link
Contributor

@LorenzMeier why was this merged? I commented that it was wrong and made a review.

@julianoes
Copy link
Contributor

@LorenzMeier why was this merged? I commented that it was wrong and made a review.

Aha, because my review was wrong. Alright, whatever.

@weekly-digest weekly-digest bot mentioned this pull request Apr 26, 2020
Closed
@TSC21 TSC21 added this to the Release v1.11.0 milestone May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianoes julianoes julianoes requested changes

@LorenzMeier LorenzMeier LorenzMeier approved these changes

@dagar dagar Awaiting requested review from dagar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release v1.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@TSC21 @dagar @julianoes @LorenzMeier