equinor / segyio

Fast Python library for SEGY files.
Other
476 stars 214 forks source link

'unable to find sorting' messages update #459

Closed raplima closed 4 years ago

raplima commented 4 years ago

Hi, I would like to suggest two small changes, both related to the "unable to find sorting" message. One in the README.md, one in the message itself. I know I spent a little longer I would like to admit trying to understand why that was happening a while ago and I know a colleague bumped in the same issue recently. I hope this modification is helpful!

jokva commented 4 years ago

I'll merge this once the compile error is fixed - you need to add an overload for runtime error with two arguments.

raplima commented 4 years ago

I think I can do that. I'm a little confused about what the function should return and PyErr_Format PyErr_SetString .

Does one of the options below work?

template< typename T1, typename T2 >
PyObject* RuntimeError( const char* msg, T1 t1, T2 t2 ) {
    return PyErr_Format( PyExc_RuntimeError, msg, t1, t2 );
}

or

template< typename T1, typename T2 >
PyObject* RuntimeError( const char* msg, T1 t1, T2 t2 ) {
    PyErr_SetString( PyExc_RuntimeError, msg , t1, t2)
    return NULL;
}

I guess another quick option for me would be to remove the references to iline and xline number from the message for now.

jokva commented 4 years ago

Usying PyErr_Format should be fine. Of course, removing the il/xl number from the message means you won't need the overload, which would also be ok (but maybe take something away from the patch).

jokva commented 4 years ago

Alright, this looks good. I'll merge this now, but squash it to a single commit so that there are no non-compiling commits in the log.

Thanks for the contribution!

raplima commented 4 years ago

Thanks @jokva! I squashed and pushed my branch. I also changed the name of this PR for something more appropriate (I didn't notice I could do that before). Let me know if I missed something

jokva commented 4 years ago

It's already merged, don't worry :--)