How should we proceed?

[kevindecapite]

@cyjoelchen

Thank you for granting me collaborator access, Joel. Before I start merging any changes, I would like to discuss a few things.

Proposed Development Cycle:

1. Developer A creates a PR.
2. Developer B reviews the PR_. (Developer A revises as needed.)
3. Developer B approves PR.
4. Developer A merges PR.

The important consideration is that the author of the PR merges the work, but only once it's been approved.

Extension Versioning:

At the moment, the README contains a version and revision number (currently @2.0, rev 28).

• What do these mean?
• Should we implement semantic versioning with tagged releases?

Looking forward to your feedback.

[aloistr]

Please add me also as collaborator. This is better than me maintaining a fork.

[kevindecapite]

@aloistr I would prefer we follow the "Proposed Development Cycle" as much as possible as it ensures a double-check on all work performed. In short, it simply means that someone else must always check the other person's work before that work can be merged into master.

This is how I manage the workflow with my team and it's very effective; it also provides a learning opportunity for the other programmer (i.e. non-author) so there can be more awareness of what's happening in the codebase as it changes.

Of course, this isn't required, but simply my recommendation/preference. Cheers.

[aloistr]

OK with me. Please explain what PR means and the precise steps to implement you procedure.

[aloistr]

Of course we should have test cases, so that we could run 'make test' in a meaningful way. That would solve a lot of problems.

[kevindecapite]

Great. I will try to explain the process more clearly. When new work needs to be done, we can:

1. Create an issue here in Github, if one isn't yet created. This explains the work requirements.
2. Make sure your local master branch is up to date.
3. Checkout new branch off master, naming it something related to the issue (perhaps even issue/123).
4. Push new branch to this remote once the work is ready for review.
5. Open a PR[1][2] (i.e. pull request) from this branch and request a review from another collaborator.
6. Wait for feedback from the other collaborator who will review and test the work; and who will provide changes that need to be made for approval.
7. Once the "feedback loop" in step 6 is completed, the reviewer will approve the PR.
8. The original author can safely merge the work into master, then delete the branch if they like.

---

1. The concept of a pull request is specific to Github and not related to git. In creating a PR, one is asking for the work in their branch to be "pulled into" the target branch. Typically the target branch is master, but it needn't be.

2. In other projects, what we do once the PR is created is reference the original issue and then close it. Github will then link the issue and PR together so, for historical purposes, we can always traverse the steps and conversation that occurred along the way, from issue to PR and back.

[kevindecapite]

Of course we should have test cases, so that we could run 'make test' in a meaningful way. That would solve a lot of problems.

Yes, I agree completely. I will create an issue for this task.

[aloistr]

i think we should differentiate about which kind of commits need review.

My push referred mostly to SwissEphemeris C code update from 2.09 to 2.10.01 The SE source code releases are well tested and grant compatibility. Such an update should by all means not create issues for rebuilding the php extension. I do not see what peer review would bring for an update on the C source side. If the extension build works, things should be ok. Such reviews must happen on the SwissEph C releases.

of course, having large set of test cases will be helpful.

Review is welcome and needed however for the php interface to new functions.

And for fixes of existing php functions, like I did with swe_rise_trans(). (Fix still needed fir its sister function)

the addition of missing #define constants is a non-critcal update, as it can hardly introduce any problems.

I am not very fluent with git, and never used branches before. I know how to make one, but do not know how to switch between them and merge back to master. I will have to learn such useful things.

[kevindecapite]

This all sounds fine to me. Please do review my PR (#14) which I referenced in the other issue as well. It fixes swe_rise_trans() and swe_rise_trans_true_hor(), but I would like to know if I did it correctly or not.

That PR also brought in some missing methods which were never implemented, and now has some conflicts due to the code which was merged earlier today. So we need to sort it all out.

[kevindecapite]

You're likely already aware of this resource, but in case not: https://git-scm.com/doc

It is distributed and atomic, which means that there are usually a high number of "duplicate" branches laying around. Once the repository (repo) is cloned, the branches in that repo are tracked on your computer, but as remote branches. You can then check any of them out which makes it a local branch. Then you can commit your work to that branch, but it's only locally committed. Pushing to the remote adds your commits (the atomic portion of Git) to the remote branch where others can fetch and merge.

A git pull is a combination of those above 2 commands.

Merging can also be done via Github itself from the PR. As always, there is a variety of ways to do the same task with this technology. I am happy to help where I can.

Edit I should mention that it's possible even to have multiple remotes, and each can have their copy (and state) of various branches. They do not need to be in sync with each other, which is one reason why the commit SHA is the most significant part of one's Git repo as it is unique and points to your code in a specific state.

[aloistr]

I suggest you do a manual merge. It was not wise to leave your pull request hanging during several months.

I would not know how to get your branch into my local git, to do the merge on my end. I need explicit command sequences, if you want to instruct me, in the form of types this type that General descriptions in git or github terminology are not of much use for me. I am not used to it. Anything you want to instruct me must be explicit, like code. For this reason, it is certainly better if you do the merge into the master branch yourself.

[aloistr]

I now merged functions swe_deltat_ex, swe_get_ayanamsa_ex_ut, swe_get_ayanamsa_ex from https://raw.githubusercontent.com/arcpointgroup/php-sweph/master into sweph.c The functions swe_rise_trans* I deem correct, they appear to work, in the current master branch.

[kevindecapite]

Thank you. I will close this "issue" as it appears a process is developing.

It was not wise to leave your pull request hanging during several months.

Agreed. In fact, I forgot about the PR as it was created before I had collaborator access, and I use my fork in production, which already has the relevant fixes. Now that this original repository is receiving the improvements it needs, it will be nice to have a single source of truth for the SE PHP extension.