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

[WIP] High level mapping elements #37

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

theangryangel
Copy link
Contributor

WIP for #18 This PR add some basic high level mapping functionality.

I've so far purposefully kept it out of the rest of the core, at least for now.

I needed to make a few modifications to core to make the plugin mounting working as I thought it was intended to be used.. If I'm wrong on that front please let me know and I'll walk those back.

A sample of what it currently looks like is at https://gist.github.com/theangryangel/54a6e8f62d326dbda8e96a670dd6dfee

I'd appreciate any feedback. It is rather basic for now, but is probably at the point where I will start using it internally and then tweaking as I go.

@nerdoc
Copy link
Member

nerdoc commented Feb 10, 2021

This seems to looks very nice, thanks for this substantial code portion. Yes, this is the way I think the direction should go - the syntax you suggest is clear to me. I can pull this PR - or let you finish it - how yo like - just tell me.

@theangryangel theangryangel marked this pull request as draft February 10, 2021 15:03
@theangryangel
Copy link
Contributor Author

I'm working on a project that I needed this for at the moment. I'm coming across a few changes I'll need to make for ergonomic reasons. If you're happy with it, I'll keep this draft for a week or two whilst I fiddle with it? :)

@nerdoc
Copy link
Member

nerdoc commented Feb 10, 2021

Sure!

@imapanda
Copy link
Contributor

imapanda commented Aug 3, 2021

How are you on the progress of this PR ?

@nerdoc
Copy link
Member

nerdoc commented Aug 8, 2021

How are you on the progress of this PR ?

that goes to @theangryangel 😄

@theangryangel
Copy link
Contributor Author

Unfortunately I’ve been retasked onto other projects and the EDIFACT things I was working on have stalled.

If anyone wants to take this and finish it please feel free.

I’ll happily close the PR if it would help keep the place tidy 🙂

In terms of the code in the PR it was working quite well, but:

  • Ergonomically it was a bit of a pain mapping out larger files. But I think that’s just the nature of the beast.
  • Validation is a bit basic/unfinished.

@nerdoc
Copy link
Member

nerdoc commented Aug 12, 2021

@theangryangel This is absolutely no problem. I have few time too, therefore development here is slow, and I only use effort here if needed. But I need this project on the long term, so I don't plan to abandon it. Nevertheless, PRs are more than welcome. Just take your time.
As said, I planned it for long term, and was impressed that anyone seems to use it at tat stage of development. But cool, that's OpenSource.

@FelixSchwarz
Copy link

@nerdoc Thank you for creating the library. A year ago I replaced some of my custom edifact code with pydifact and it just became more robust+featureful. Since then some of our edifact recipients complained about invalid data but the problem was always on their side: Their edifact parser were just too limited to parse our perfectly valid data :-)

@theangryangel
Copy link
Contributor Author

FWIW we've got internal resource looking at this again, I'm not sure where it's going to go atm, but if you want us to close the PR because it's too noisy, let us know <3

@nerdoc
Copy link
Member

nerdoc commented Oct 20, 2022

@theangryangel Don't stress, I have none. It's so cool that this library which I started to maybe use sometimes - is already used by some people - and I love that open source spirit. So just take your time - we'll keep this PR open as long as you need it.

- added delfor and deljit validation
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.

5 participants