Skip to content

Conversation

@lvrfrc87
Copy link
Collaborator

@lvrfrc87 lvrfrc87 commented Nov 19, 2021

As part of refactoring, this is the first of a long PR series to make the code more readable.

All the utilities functions are now migrated under utils folder. Test are also added for each single utils.
Var and functions are also renamed and comment lines are also added.

More refactoring is needed as for example possible implementation for utilities under a single class, as well as improve the overall library logic. Those will follow after this PR as I first need to make the code more readable and have all the tests in place

Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

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

not strong opinion, but I would simplify to netcompare/utils/ instead of an extra nested package for each group of functions.
Same for tests, I usually prefer to put all tests together, even in subfolders:
tests/check_type.py & tests/utils/check_refkey.py ...

@lvrfrc87 lvrfrc87 changed the title Move jmesparser under utils WIP:Move jmesparser under utils Nov 22, 2021
@lvrfrc87 lvrfrc87 marked this pull request as draft November 22, 2021 09:22
@lvrfrc87 lvrfrc87 changed the title WIP:Move jmesparser under utils WIP: Move jmesparser under utils Nov 22, 2021
@lvrfrc87 lvrfrc87 changed the title WIP: Move jmesparser under utils WIP: Create utils for library refactoring Nov 22, 2021
@lvrfrc87 lvrfrc87 changed the title WIP: Create utils for library refactoring WIP: Create utils for library refactoring Nov 22, 2021
@lvrfrc87 lvrfrc87 marked this pull request as ready for review November 22, 2021 14:13
@lvrfrc87 lvrfrc87 changed the title WIP: Create utils for library refactoring Create utils for library refactoring Nov 22, 2021
chadell
chadell previously approved these changes Nov 23, 2021
Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

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

good one. just some naming "suggestions"

@lvrfrc87
Copy link
Collaborator Author

lvrfrc87 commented Dec 6, 2021

@grelleum I am doing some refactoring in the code. I have managed to make all the test work apart the nested list implementation. Is something I can look at tomorrow but I was wondering if you can spare 10 minutes on it as you can quickly spot the issue and speedup the work.

@chadell considering there is some work to do in matters of comments and doc string, would you mind to have a fresh look at this PR??

@lvrfrc87
Copy link
Collaborator Author

lvrfrc87 commented Dec 7, 2021

@grelleum Nevermind, I fixed :)

Copy link
Collaborator

@pszulczewski pszulczewski left a comment

Choose a reason for hiding this comment

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

Needs improvements mainly in type hints.

@lvrfrc87
Copy link
Collaborator Author

lvrfrc87 commented Dec 8, 2021

I will go ahead and merge this PR so we can use an updated version of master for the coming PRs

@lvrfrc87 lvrfrc87 merged commit e46b79f into main Dec 8, 2021
@lvrfrc87 lvrfrc87 mentioned this pull request Dec 8, 2021
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.

6 participants