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

Add ASN data source #285

Merged
merged 3 commits into from
Nov 4, 2022
Merged

Conversation

kyle-burnett
Copy link
Contributor

  • Adds netbox_asn data source

@fbreckle
Copy link
Collaborator

fbreckle commented Nov 3, 2022

Hi,

thank you for your MR. This is looking quite good already. I am a somewhat unsure about the naming of the query attributes:

You are the first person to implement an exclude filter. This means we have no existing convention on the naming of these attributes.

First, since "positive" filters are the default, I think you should rename tag_include to just tag.

Second, for other "lookup expressions" (full list here), we have to decide if we either use a "human friendly" word, like you did with exclude for __n (although I personally would choose not), or do we use the technical suffix directly. In this concrete example, excluded tags would be tag__n. The explanation of the suffix should then be part of the description of the attribute, including a link to the page above.

Since a human friendly string for things like niew Does not end with (case-insensitive) is either long or cryptic, I think we should use the latter approach.

Opinions?

@kyle-burnett
Copy link
Contributor Author

Thanks @fbreckle for the feedback.

I agree that using the explicit suffixes in the attribute names makes more sense than "human friendly" names. Especially when you start to consider the other suffix filters like the example you provided.

I updated the include and exclude attributes to be tag and tag__n, respectively.

I see you noticed that I used the same naming convention in #284. If this looks good to you now, I can replicate the changes in that PR.

Thanks!

docs/index.md Outdated Show resolved Hide resolved
@fbreckle
Copy link
Collaborator

fbreckle commented Nov 4, 2022

LGTM now, just wondering about the docs part.

About the lookup expressiosn: I might end up changing my mind about these. The arguments above are valid, but my concern is that it is really unreadable when reading code that was written by someone else. Maybe long, but human readable mappings are better after all.

The good thing is, refactoring/deprecating these expressions should be rather easy, as long as they are not everywhere yet. We could even just allow both.

This is just FYI. I will start a discussion about this when I have time. For now, we go with the technical names.

@fbreckle fbreckle merged commit b364e39 into e-breuninger:master Nov 4, 2022
twink0r pushed a commit to twink0r/terraform-provider-netbox that referenced this pull request Sep 15, 2023
* Add ASN data source

* Change naming standard for tag variables

* revert index.md change
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.

2 participants