From: "Hannes Dürr" <h.duerr@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM
Date: Tue, 10 Dec 2024 10:32:52 +0100 [thread overview]
Message-ID: <9553725a-a0a4-44e4-87f6-cc5d2d347424@proxmox.com> (raw)
In-Reply-To: <mailman.781.1732724843.391.pve-devel@lists.proxmox.com>
Thanks for contributing and sending the patch series, we really
appreciate it!
At first glance it looks quite good, I have a few suggestions for changes:
* The plugin is based on the Netbox plugin, I would suggest changing it
to the base plugin. I know Nautobot is a fork of Netbox, but this
dependency doesn't make things easier in my eyes, unless I'm missing
something, please let me know.
* The commit history is currently a bit unnecessarily long and does not
build up well. What I mean is:
- in 1/12 you build the plugin and copy/paste some stuff into it
- in 3/12 you delete everything for now
-> just leave it out
or
- in 6/12 you introduce a typo in an url
- in 7/12 you fix the typo
Such changes don't need to be committed in the first place,
so you don't need to fix them 2 commits later.
* The commit messages could be a bit longer and more explanatory for
coming changes: e.g. “api endpoint change no longer needed” -> Why is
the endpoint change no longer needed? Is there more background
information on this, e.g. a link?
* Comments can be helpful to make complex code easier to understand
after a long time. Comments like '#helper' or '#impl' are not really
necessary in my opinion. I know our codebase already contains such
comments elsewhere, but I think we can leave them out here :)
* Apart from the tests, the documentation (pve-docs/pvesdn.adoc) is also
missing.
On 11/27/24 17:17, Lou Lecrivain via pve-devel wrote:
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next prev parent reply other threads:[~2024-12-10 9:32 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-27 16:17 Lou Lecrivain via pve-devel
2024-12-10 9:32 ` Hannes Dürr [this message]
2024-12-10 13:19 ` Lou Lecrivain via pve-devel
[not found] ` <BE1P281MB2116C1CA8BA34E5C86FA59D9853D2@BE1P281MB2116.DEUP281.PROD.OUTLOOK.COM>
2024-12-12 15:42 ` Hannes Dürr
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9553725a-a0a4-44e4-87f6-cc5d2d347424@proxmox.com \
--to=h.duerr@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox