About these changes, see b135229

See a9db13e, this was done twice.

This is still necessary if you want to instantiate an HDU instance directly (e.g. with type.__call__) and have it return the correct HDU class based on the header. I don'e think that the existence of _readfrom_internal obviates its need because that's only used in alternative constructors for reading an HDU from data _BaseHDU.fromstring and _BaseHDU.readfrom. That's different from calling _BaseHDU.__new__ directly.

Yeah I realized this later with #8660, as some convenience methods relies on this. In #8666 I added a new class method to use the HDU class detection, i.e. _BaseHDU._from_data(data, header) instead of relying on _BaseHDU.__call__. I think it's a good compromise, it is no more possible to have an HDU class change is type based on the header which is a good thing imo, and we avoid the duplicated detection in the general case.

Aren't headers supposed to be ASCII?

Yes, they are supposed to. But sometimes they contain unicode chars, which will fail here, and fallback to Header, which then use decode_ascii.
One other option, that I tried, would be to keep working on bytes here. And store the header string as bytes, which would save some memory and avoid decoding headers that are not used later. But there are some complications, like the need to check for padding bytes, and the gain was null... but I will probably try again this later.

Ins't the = separator always supposed to be in a specific character position? find could be slow... Ah I see what comes below. What about, to speed things up in the most common case, making the start of the 'if' statement if card_image[8] == '=' and only using find if that fails?

Good idea, I will try this.

find supports start and stop, maybe we can use these to limit the "searched region": card_image.find(VALUE_INDICATOR, 0, 9) (not sure if the indices are correct ... 😓)

Good to know. I already implemented something with @astrofrog 's suggestion (and forgot to push it ...), where I slice the first 8 characters before the .find. Not sure if it makes a big difference with using the start and stop parameters.

Not sure if it makes a big difference with using the start and stop parameters.

It's probably faster because it avoids the copy. Probably not big but worth a try

It seems to make a small difference, always good to take 👍

I would prefer the old style here.

Why, multiple assignment is cool :p

I agree with @saimn!

😱

Maybe it's just me, but it feels like an unintended misspelling of value = self._value == self._parse_value(). Also it reminds me of C (not in a good way 😄).

Don't get me wrong, I won't start a fight about it - but this looks so weird to me because it's so rare that I see this in Python and since assignment is normally a statement not an expression it looks like it really shouldn't work.

I don't know, I frequently use this, typically to get a shorter variable name for a local block and avoid the lookup time on self. It's quite practical, and imo easier to understand that it references the same variable.

Okay, so it's probably just a matter of getting used to it. I think we probably have to get used to it when Python 3.8 with PEP 572 -- Assignment Expressions comes.

wow, I should read more PEP, this looks quite interesting (though I agree that feels very unusual)

Ah yes, PEP 572, the one that led to Guido stepping down from BDFL! I thought it was strange at first but I constantly find myself needing it.

I actually though cdef list - it's amazing that it worked 😮

Oups! It wasn't done on purpose but yeah it seems that cython understand this. The generated code is slightly different:

+14:     cdef read_blocks = []

  __pyx_t_1 = PyList_New(0); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 14, __pyx_L1_error)
  __Pyx_GOTREF(__pyx_t_1);
  __pyx_v_read_blocks = __pyx_t_1;
  __pyx_t_1 = 0;
...
+29:         read_blocks.append(block_str)

    __pyx_t_9 = __Pyx_PyObject_Append(__pyx_v_read_blocks, __pyx_v_block_str); if (unlikely(__pyx_t_9 == ((int)-1))) __PYX_ERR(0, 29, __pyx_L1_error)

and with cdef list:

+14:     cdef list read_blocks = []

  __pyx_t_1 = PyList_New(0); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 14, __pyx_L1_error)
  __Pyx_GOTREF(__pyx_t_1);
  __pyx_v_read_blocks = ((PyObject*)__pyx_t_1);
  __pyx_t_1 = 0;
...
+29:         read_blocks.append(block_str)

    __pyx_t_9 = __Pyx_PyList_Append(__pyx_v_read_blocks, __pyx_v_block_str); if (unlikely(__pyx_t_9 == ((int)-1))) __PYX_ERR(0, 29, __pyx_L1_error)

Why was this moved?

Because it must be defined before self.value is set. Using the new header class triggered this bug (b135229) in the getter/setter loop for .value, and the same for .comment.

Seems odd to raise a plain Exception. That way we probably swallow any exceptions that we don't expect when catching it.

I don't have a good idea though what makes sense here. Maybe ValueError or some custom exception like FastHeaderParsingFailedException? 😄

In any case there is a fallback to the Header parsing. I'm fine with using a custom exception but this will not make a difference, we need to catch any exception in the fast reader and then call Header as it have a better reporting of errors.

Maybe the END check should come before the find call? That is, first check if card_image[8:10] is the value indicator, then check if the card is the end card, then do the thing with find?

Why?