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

Add georec, mxv, radrec, spkezr, vcrss, vdot, xpose #1

Closed
wants to merge 9 commits into from

Conversation

s-rah
Copy link
Contributor

@s-rah s-rah commented Sep 13, 2021

No description provided.

@GregoireHENRY
Copy link
Owner

GregoireHENRY commented Sep 13, 2021

Dear @s-rah

First of all, thank you for your pull request!!
You're the first one that I receive and I'm learning from it.

I've been off the development of rust-spice for a bit as I got from it everything that I needed. But like you can see, many functions are still missing. Thank you for your changes, bringing more functions and more excitement to me to continue this project.

Before talking about the PR, I got couple of questions:

  • how did you find the project structure?
  • does the project structure make sense?
  • what would you change in the project structure?

More questions concerning the PR:

  • did you run the tests of the project before submitting PR?
  • if you ran the tests, went they well?

Status of your PR:
I think I should not accept the PR in this state. You requested it to be on the "main" branch, which is 8 commits behind the "dev" branch. These commits are fixing a bug on "main" that I found lately about FFI with C strings.

I should have written a contribution document with my expectations, I'm sorry for that I'll do it immediately.

Is it possible that you re-publish your pull-request on the "dev" branch? You will notice some changes about how to defines input and output strings.
After you new PR, we can merge the "dev" branch into "main".

Let me know if this plan fits with you. I'm open to any remark and advice.

@s-rah
Copy link
Contributor Author

s-rah commented Sep 13, 2021

Hi!

Thank you for this project.

how did you find the project structure?
does the project structure make sense?

The project structure is great, it was very quick to get going.

what would you change in the project structure?

Currently the packaged metakernel (rsc/krn/hera_study_PO_EMA_2024.tm), which most of the tests use, references "/home/greg/rob/hera/kernels" which means they fail out-of-the-box (I ended up overriding the file locally) - it is probably worth considering packaging a standalone test kernel for the tests with the relevant files so they run nicely on their own.

Some of the -derive annotations like (return_output) required some experiment to work out what they did and when they should be included.

did you run the tests of the project before submitting PR?

Yes

if you ran the tests, went they well?

Yes (with the caveat above about locally overriding the kernel)

Is it possible that you re-publish your pull-request on the "dev" branch?

Makes sense. Will do.

@s-rah s-rah force-pushed the main branch 2 times, most recently from 0ec95fb to 533a499 Compare September 13, 2021 16:38
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.

2 participants