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

Refactor project structure #55

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

muff1nman
Copy link
Contributor

This is an effort that I just started, but I wanted to open up this PR for any suggestions/comments/concerns as the process is underway. My intent is to make the project compatible with setuptools and break the code base into separate modules.

@mhagger : if this is something you'd prefer to lead, I understand.

@muff1nman
Copy link
Contributor Author

I think it would be prudent to add more tests. Any thoughts on adding support for a testing framework?

@mhagger
Copy link
Owner

mhagger commented Mar 2, 2014

This is awesome! I'd thought about doing this but haven't had time to get to it. So it's ✨really great✨ that you are working on this.

I do have one wish: that when you are done, before you ask me to merge you rebase your commits into a logical sequence, such that each commit is self-contained and, for example, commits like "remove extra newlines" are squashed into the commit that introduced the extra newlines. I can help you with this tidying up if you are not into that sort of thing. (As long as you are working on the PR feel free to commit in whatever order you want and rebase at will.) I will try to keep an eye on whatever you push and give feedback when I think it might help.

@muff1nman
Copy link
Contributor Author

Absolutely. Ill try my best to clean up the history every now and then.

@mhagger
Copy link
Owner

mhagger commented Mar 2, 2014

A note about version numbers:

It's really important that we start defining version numbers for git-imerge. One reason is because packagers need them (e.g., see #54), and it is in our interests to help packagers. The other is that it will make it easier to support users if we can ask them "what version of git-imerge are you using?"

setuptools requires a version number too, doesn't it?

The bigger picture of managing version numbers needn't necessarily be part of your project, but in case you had plans in this direction, please see #54, where I wrote down some thoughts about how we could manage version numbers in a sane way.

@mhagger
Copy link
Owner

mhagger commented Mar 2, 2014

I think it would be prudent to add more tests. Any thoughts on adding support for a testing framework?

Absolutely! Are you trying to make me fall in love with you or something, because I should warn you that I'm already married 😉

But adding a testing framework should be done as part of a separate PR, right?

@muff1nman
Copy link
Contributor Author

Ah. I must have skipped that attribute in the setup.py. Ill add it.

I like the link you had in #54 although my thoughts are that we should start simple and only add something like that if we find we need the extra granularity. I have been looking around at other python projects and here is another idea.

Absolutely! Are you trying to make me fall in love with you or something, because I should warn you that I'm already married 😉

Haha. Frankly Im afraid to start moving code around until I feel like there are some tests that will protect the status quo a bit.

@muff1nman
Copy link
Contributor Author

Hmm. Yes I would agree that separating out tests into a different PR would be sensible. It should probably be done first. I'll see what I can do.

Copy link
Contributor

@jdufresne jdufresne left a comment

Choose a reason for hiding this comment

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

Nice! I was thinking about the same feature.

I think we should also add a __main__.py to the package so it can be executed as a module: python -m gitimerge ....

https://docs.python.org/3/library/__main__.html

For a package, the same effect can be achieved by including a main.py module, the contents of which will be executed when the module is run with -m.

from gitimerge.gitimerge import main
import sys

main(sys.argv[1:])
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this file shouldn't exist and instead be built using the setuptools console_scripts entry point feature. It is more compatible and tested across platforms.

https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-console-scripts-entry-point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants