Skip to content

Conversation

tammoippen
Copy link
Contributor

Hi @nerdoc,

I am using your project and would like to improve on the typing situation. Along with that, I

  • removed SegmentCollection, FileSourcableMixin and UNAHandlingMixin, as that improved typing a lot (and they were to be dropped in v0.2 anyway)
  • increased the minimum python version to 3.9, as 3.8 is EOL since September 2024. (https://devguide.python.org/versions/)

Best and thank you for your effort.

@lord-haffi
Copy link
Contributor

lord-haffi commented Apr 4, 2025

Actually stumbled across this issue right now; funny that you just opened a PR to support proper typing ^^
Big thumb up from me 👍

recipient: Element,
control_reference: Element,
syntax_identifier: Element,
syntax_identifier: tuple[str, int],
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to move here to an even more specific Element, but tuple[str, int] is ok for me for the moment.

@nerdoc
Copy link
Member

nerdoc commented Apr 8, 2025

Great, thanks for the improvements, that's a thing I always wanted, too.
Since we are dropping py3.8 support now (god thanks), I would even go farther and remove "typing" imports, and all the List, Tuple etc. stuff.

from typing import List, Tuple

foo: List[int]
bar: Tuple[str, int]
baz: Type[int]

could be written much easier as

foo: list[int]
bar: tuple[str, int]
baz: type[int]

with Python builtins.

The only things that remain (until python 3.10) are the Union and Optional annotations (which I hate too).

pydifact has been proven stable for a longer time now, and people who are using python 3.8 because their company thinks it must stay in 2024, can use earlier pydifact versions too IMHO, but I'd like to go onwards.

So why not drop 3.9 support too, security EOL is 31 Oct 2025, that's almost tomorrow.

And with 3.10 we could write int | None instead of the bulky Optional[int]

What do you say? I'm fine with it, so If you want you can just add this as additional commit and remove all the ballast of Optional, Union, and python 3.9 dependencies in .github and .travis.yml files.
(I did that for 3.8 in 3 separate commits online on Github.com, that's why it's 3...)

def test_split_by():
def _serialize(collections: Iterable[RawSegmentCollection]) -> List[List[str]]:
lst = []
lst: list[str] = []
Copy link
Member

Choose a reason for hiding this comment

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

this is python 3.10 anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://peps.python.org/pep-0585/ is available since 3.9.

@tammoippen
Copy link
Contributor Author

Hi @nerdoc, I adjusted for python 3.10. Best, Tammo

@nerdoc
Copy link
Member

nerdoc commented Apr 8, 2025

looks very good, I'll check the code a bit, as I in parallel made some changes in other branches that go into the same direction, but differ a bit. Precisely, I thought about creating a factory not only for segments, but also for data elements - and classes for them to make more "typed" parsing. But that's maybe too much memory it will consume.
And for my part, I am working on the support of more than one EDIFACT version - ATM it's 3/4 which is supported, but in my case I need to support version 1 (here in Austria, we live in the 90ies, in medicine).

Side note: I am looking for an official, machine readable document that describes the edifact format, so pydifact can parse it and validate the Segments it reads in against it. I have already written to the UN, but I get no answer besides the documents on unece.org - which are written for humans. No XSD schemata, no JSON formats, nothing. Just plain prosa, and tables. Maybe I'll open an issue for that, if anyone wants to join there: #81

@lord-haffi
Copy link
Contributor

@nerdoc Just a friendly reminder for this PR :)
I think "creating factories for data elements" is another problem worth to solve it in another PR, isn't it? Also, I would very appreciate proper typing here for my own projects using your library ^^ <3

@nerdoc
Copy link
Member

nerdoc commented Apr 30, 2025

I'll merge the PR in the next few days. Thanks for the reminder ;-)
Just want to cross-check if my other branch (see #81) is compatible with this code.

@nerdoc nerdoc merged commit 3a9e253 into nerdocs:master May 3, 2025
9 checks passed
@nerdoc
Copy link
Member

nerdoc commented May 3, 2025

Thanks for your great quality improvement.

This was referenced May 4, 2025
@nerdoc
Copy link
Member

nerdoc commented May 4, 2025

@tammoippen one thing: in

assert isinstance(string, str), "%s is not a str, it is %s" % (
you inserted an assert. I removed this (commit follows) again, as it breaks things. I changed the way DataElements are put together during parsing, so there are not always strs in the Segment elements, but DataElement subclasses that behave like a string.

So ducktyping should suffice here. I don't know why you added this assert, but I can imagine you had problems that "sometimes" it was no str, and you wanted to make sure. If you still experience this, please start a new issue, so we could address that directly.

@tammoippen tammoippen deleted the typing branch May 4, 2025 11:46
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.

3 participants