Skip to content

Conversation

@jtdub
Copy link
Contributor

@jtdub jtdub commented Aug 7, 2021

Copy link
Collaborator

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

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

Does it make sense to sort this? Obviously that doesn't change the code much, but thinking for readability of the code.

@itdependsnetworks
Copy link
Contributor

Yes, I agree. I guess sort by number?

@jtdub jtdub requested a review from jeffkala August 9, 2021 18:58
Copy link
Collaborator

@qduk qduk left a comment

Choose a reason for hiding this comment

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

I think this will definitely be useful. Thanks for the PR!

Copy link
Collaborator

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks for sorting it.

@jtdub
Copy link
Contributor Author

jtdub commented Aug 9, 2021

You're welcome. :)

@itdependsnetworks
Copy link
Contributor

Actually, what did you do when there was a different tcp vs udp?

doip-data | 13400 | tcp | DoIP Data
doip-disc | 13400 | udp | DoIP Discovery

@jtdub
Copy link
Contributor Author

jtdub commented Aug 9, 2021

@itdependsnetworks

I didn't take that into account, but I can!


import csv

TCP_PORT = {}
TCP_NUM_TO_NAME = {}

with open("service-names-port-numbers.csv") as f:
    reader = csv.reader(f)
    for row in reader:
        if row[0] and row[1]:
            try:
                key_exists = TCP_PORT.get(row[1])
                if not key_exists:
                    TCP_PORT[int(row[1])] = row[0].upper()
            except ValueError:
                continue

for k in sorted(TCP_PORT):
    v = TCP_PORT[k]
    TCP_NUM_TO_NAME[k] = v

print(TCP_NUM_TO_NAME)

@jtdub
Copy link
Contributor Author

jtdub commented Aug 9, 2021

I broke out the individual protocols per @itdependsnetworks catch.

@jeffkala jeffkala mentioned this pull request Aug 18, 2021
@jeffkala
Copy link
Collaborator

Didn't realize pkg_resources was in the standard library, so this is all fixed now. I just checkout'd out the PR and ran the tests and it works.

jeffkala
jeffkala previously approved these changes Aug 27, 2021
Copy link
Collaborator

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

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

Looks good now, tested it locally and seems to work as expected.

@jeffkala
Copy link
Collaborator

Didn't realize pkg_resources was in the standard library, so this is all fixed now. I just checkout'd out the PR and ran the tests and it works.

Realizing this is not actually the case, appears pip installs this. When i setup a new venv without pip this isn't importable.

@jeffkala jeffkala dismissed their stale review August 27, 2021 21:12

not approved now that pkg_resources is not std lib

@jeffkala jeffkala self-requested a review August 27, 2021 21:13
Copy link
Collaborator

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

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

Think this gets us back to the original solution to just load the JSON file @jtdub , this is what we'd prefer to avoid adding dependencies.

Update as needed if I missed anything.

@jtdub jtdub requested a review from jeffkala August 28, 2021 22:22
@itdependsnetworks itdependsnetworks merged commit f5317af into networktocode:develop Aug 29, 2021
@itdependsnetworks
Copy link
Contributor

Thanks!

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.

4 participants