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

Create package publishable to pip #22

Merged
merged 13 commits into from
Nov 12, 2018
Merged

Create package publishable to pip #22

merged 13 commits into from
Nov 12, 2018

Conversation

Williangalvani
Copy link
Member

@Williangalvani Williangalvani commented Nov 1, 2018

This refactors the packages to follow Python coding guidelines before plublishing.

brping/ping1d.py and brping/pingmessage.py will be rewritten by the script in ping-protocol, updated in bluerobotics/ping-protocol#106

Fix #18

@Williangalvani Williangalvani force-pushed the createmodule branch 2 times, most recently from cdb90dd to cf416f0 Compare November 5, 2018 22:44
@Williangalvani
Copy link
Member Author

@Williangalvani Williangalvani force-pushed the createmodule branch 2 times, most recently from bf1bf70 to f8b1f19 Compare November 7, 2018 13:59
@Williangalvani Williangalvani changed the title [WIP] Create package publishable to pip Create package publishable to pip Nov 7, 2018
README.md Outdated Show resolved Hide resolved
@patrickelectric
Copy link
Member

Is there commits here that are from generator-python.py script ? If so they should be marked as such. Also, individual commits for generated code does not make sense, they should be all together. Should this PR be merged after any PR in ping-protocol ?

@Williangalvani
Copy link
Member Author

Is there commits here that are from generator-python.py script ?

No, things were done manually here and then at the ping-protocol templates.
The newly generated file create a lot of hard to read diffs as things get moved around.
We should look into ordering things before generating the code, but that is out of the scope of these two PRs.

Should this PR be merged after any PR in ping-protocol ?

Yeah, preferably. otherwise I can just discard some of the commits regarding ping1d.py and pingmessage.py

@Williangalvani Williangalvani force-pushed the createmodule branch 5 times, most recently from 33d65bf to 32a6aac Compare November 7, 2018 18:16
@Williangalvani
Copy link
Member Author

Williangalvani commented Nov 8, 2018

@jaxxzer can you look at this? I'd like to merge this one before bluerobotics/ping-protocol#106

It is shorter than it looks!

README.md Outdated
- Using 'pip'

```sh
$ pip install bluerobotics-python
Copy link
Member

Choose a reason for hiding this comment

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

pip install bluerobotics-ping?

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch

@jaxxzer
Copy link
Member

jaxxzer commented Nov 9, 2018

I think it makes more sense to rebase this on the generated files of bluerobotics/ping-protocol#106.
I don't think it's a good idea to commit manual edits to the generated files here. I understand the workflow that you used to get to this point, but let me know if there is some problem to rebase this.

@Williangalvani
Copy link
Member Author

I'll do that then, no problem.

@Williangalvani
Copy link
Member Author

Williangalvani commented Nov 9, 2018

@jaxxzer that is going to fail, because the CI script is going to try to move the files to brping/, which doesn't exist yet.
I can just drop the commits changing these files instead (but leave the ones changing the directory structure.)

I would drop 6b466ce
and 01e2aef

@jaxxzer
Copy link
Member

jaxxzer commented Nov 10, 2018

I think we should:

  1. Make the history in this pr look like this branch: https://github.com/jaxxzer/ping-python-1/tree/pep
  2. Merge thispr (still have to review it)
  3. Merge PEP8 corrections/cleaning ping-protocol#106
  4. Rebase/Merge Necessary updates to auto update gh-pages from travis #21

@Williangalvani If you agree, can you take care of #1

@Williangalvani
Copy link
Member Author

@jaxxzer sounds good.
the instructions still need to be fixed, it says pip install bluerobotics-python instead of pip install bluerobotics-ping

@jaxxzer
Copy link
Member

jaxxzer commented Nov 10, 2018

Ok @Williangalvani please take care of the force push here when you are comfortable with it, and I will proceed with the review. You can take care of the current fixups now or after I'm done with the review.

@jaxxzer jaxxzer closed this Nov 10, 2018
@jaxxzer jaxxzer reopened this Nov 10, 2018
@Williangalvani
Copy link
Member Author

updated.

@jaxxzer jaxxzer merged commit 8d2e0db into master Nov 12, 2018
@jaxxzer jaxxzer deleted the createmodule branch November 12, 2018 20:35
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.

3 participants