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

Ipn templates #46

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

Ipn templates #46

wants to merge 14 commits into from

Conversation

cmidgley
Copy link
Contributor

@cmidgley cmidgley commented May 5, 2024

Adds support for setting IPN based on Jinja2 templates. See #38 for background, and the updated README for feature details.

@30350n
Copy link
Owner

30350n commented May 8, 2024

Okay, I quickly ran pre-commit on this to fix the formatting and also did some light refactoring:

  • changed ipn_template to ipn_format (to make it more in line with the existing part_selection_format config var)
  • changed --ipn options from "never", "new", "always" to "false", "true", "overwrite" (to make it more in line with the --interactive settings, functionality is the same repectively)

@30350n
Copy link
Owner

30350n commented May 8, 2024

Okay, so some general thoughts on the implementation:

  • Is it really necessary to use jinja2?
    I'd highly prefer an approach that just uses the standard str.format(...) function.
  • It's nice that you already added extensive documentation for the feature, but I think it could be cut down a little bit. I'd prefer one or two sentences in the config.yaml section, with a simple example. (Let's do this last though, once we've settled on a final implementation).
  • One thing I don't like is that this runs pre parameter-normalization. This is pretty my fault though, because it currently just gets done in setup_parameters(...). I'll refactor this on master by creating splitting this up into a match_parameters(...) function which adjusts the parameters on the api_part directly and will be run before committing the parameters to InvenTree. Then your import_part_ipn can be run inbetweeen those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants