DO NOT MERGE

[ferruzzi]

Figured I'd try this as a way to comment on your code without getting too confusing. I'll make some changes and upload that as a separate file so you can see how I might have done it.

Overall, mostly just style suggestions.

[ferruzzi]

As a general rule, these should be inside a method. There isn't really any reason to have commands outside of a method. Save the top up here for declaring constants, and everything else gets wrapped up tidy.

[ferruzzi]

By convention, names of constants are always in all caps

Suggested change

	# for now just to name the file / Soon to add for user to add input :)
	whid = 'YEG1'
	WHID = 'YEG1'

[DouglasVSegatto]

okay, even if later I'll make it to ask user the "whid "input, I should make it all capital to mean it should not changed, right?

[ferruzzi]

I'm actually talking about that in the other file I'm going to upload. it's an interesting one. In general, the value should never change so I'd say yes, all caps. BUT from the other point of view, it IS changing once because it is being set. In the end, it could go either way, I guess.

[ferruzzi]

This would get moved into "main" when you implement it.

[ferruzzi]

By convention, starting a method/function name with an underscore shows others that it's not meant for them to use. If someone builds on your package, would you expect them to want to use this function? If so, then this should return as you mentioned below. if not, then start it with an underscore and it can do whatever it wants because it's "private".

In this case, it's up to you. but defaulting to an underscore is never a bad idea.

[ferruzzi]

Looking at the code for send_command, use_textfsmis a boolean so it expects True or False. I suspect you meant to use textfsm_template there?

You didn't get any errors here because Python is a "Truthy" language. When it expects a boolean and it gets pretty much anything, it assumes that is True. It's smart enough to know that an empty string is False, and the integer 0 is false, but the string "banana" or any number other than 0 is treated as a True.

[DouglasVSegatto]

not sure if i get your question right, but zthe output from this command will be formatted as the textfsm template, which should look just like json.

here is the output how it looks like (ps: i added just 2 lines/interfaces output but it could have many)

output = [
{
    "iface": "Te1/0/39",
    "temperature": "36.1",
    "voltage": "3.29",
    "tx_pwr": "41.8",
    "rx_pwr": "-2.5"
  },
  {
    "iface": "Te1/0/40",
    "temperature": "35.1",
    "voltage": "3.30",
    "tx_pwr": "36.5",
    "rx_pwr": "-2.1"
  }
]

[ferruzzi]

https://ktbyers.github.io/netmiko/docs/netmiko/base_connection.html#netmiko.base_connection.BaseConnection.send_command

In their documentation, they say that use_textfsm is a boolean (True or False) and it defaults to False. So your string '~/ntc-templates/ntc_templates/templates/cisco_ios_show_interface_transceiver.textfsm' that you have there is only being read as "True". I think what you meant was:

Suggested change

	use_textfsm='~/ntc-templates/ntc_templates/templates/cisco_ios_show_interface_transceiver.textfsm')
	transceiver_output = connection.send_command(
	'show interface transceiver',
	read_timeout=90,
	use_textfsm=True,
	textfsm_template= '~/ntc-templates/ntc_templates/templates/cisco_ios_show_interface_transceiver.textfsm'
	)

[ferruzzi]

Suggested change

	connection = netmiko.ConnectHandler(host=host, device_type='cisco_ios', username=username,
	password=password, secret=password)
	connection = netmiko.ConnectHandler(
	host=host,
	device_type='cisco_ios',
	username=username,
	password=password,
	secret=password
	)

[DouglasVSegatto]

this one i was lazy, i did this way on the test but didn't change here

when first writing script/code for me straight line felt easier

[ferruzzi]

All in one line is fine if it fits and you like it. My team uses 120 character as the maximum line length, as a guide for "fits on one line"

[ferruzzi]

It's considered poor form to catch Exception, you should always be as explicit as possible here so you don't accidentally swallow an error you didn't mean to.

It looks like ConnectHandler and enable could raise ValueError, send_command can raise ReadTimeout. You could replace except Exception with except ValueError, ReadTimeout, or you could catch them separately, depending on whether you want to take the same action for all possible failures. For example, perhaps a ReadTimeout could be a network issue and you want to retry once before giving up.

[DouglasVSegatto]

my idea was to have the device and error shown, which i could later come back and check the error from the output and try is later.

but yeah its a good idea to have it retry depending on the error! noted!

[ferruzzi]

Both of these are true statements :P

Also, by convention, every python script/file should end in a blank line. The reason for it is "old technology", it's a throwback to when most people used a terminal. when you cat the python file, having a blank line at the end of the file makes it a little easier to read. But either way it's a standard that pretty much everyone does, and many linters will enforce it.

Which brings me to Linters. You should install one. A linter is a script that will go through your code and clean it up. Depending on your settings it can do thins like make sure you always use the same quotes (python doesn't care if you use 'single' or "double" quotes, but it's generally expected that you will pick one and stick with it) and it can make sure you have the right number of blank lines before a method or class, etc. most of those things they can even fix for you magically, so you just run the command and it makes your code pretty. my team uses ruff for most things: https://beta.ruff.rs/docs/tutorial/

[ferruzzi]

This is generally a bad idea. below you used the with context manager like this:

with open(file, "r"):
    stuff....

And that is the "right" way. One of the main problems with this way here is that you never closed the file. If someone else tried to open that file even if you aren't currently running your script, they could run into errors because it's locked by you. The operating system should eventually realize your script is done and eventually unlock it, but it's a bad idea to cross your fingers and hope.

The context manager magically closes the file when the code leaves the manager (hits the unindent) so that is never an issue.

Which generally also means it should be inside a function or method. But you already know that :P

[DouglasVSegatto]

I actually didn't know it was a best practice just felt right to move it to a variable and work with that after(LOL)

You mentioned "the context manager magically closes .." do you mean the way i did it, is good, it will always close automatically after "moving" the content to the variable, right?

just confirming if it go it right lol

[ferruzzi]

You used a context manager the second time, down when you saved the data.

This is a context manager:

with open(file, mode) as file:
    do_stuff()
    
do_more_stuff()

After do_stuff(), the manager will close the file before moving on to do_more_stuff().

[ferruzzi]

Nah, using append is perfectly fine here. I suspect the reason it was overwriting is because append() returns None. if you wanted this to return a value then it should return (host, (sfp['iface']), pwr, state, model, speed) and below where you call it, it would be something like formatted_data.append(light_level_check(connection))

[DouglasVSegatto]

I can't roll back to what i did to confirm, but i do believe it did exactly what you suggested,but if you think the way it is, looks good and not like a "remediate way to make it work" then should be fine ;)

[ferruzzi]

I added a file called day1-rewrite.py. You can go through it and leave comments just like I did above. When you write a comment, pick "add to review" and then at the end submit the review. If you submit each comment as individual comments, then I can't reply. It's a github issue.

Click on "files changed" at the top to see it.