-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
Full models audit #2503
Full models audit #2503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick first review, I didn't check the query module yet.
Just thinking out loud: I get that you wanted to separate the data collection from the output, but since the logic is already in the query module, wouldn't it be better to group the the data variables to the related frame and then split each group to its own function?
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
extensions/pyRevitTools.extension/checks/modelchecker_new_check.py
Outdated
Show resolved
Hide resolved
I still need to review the numbers outputed and maybe get to solve why the document is not properly stored / cached |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's time to roast the config module 😅
...sions/pyRevitTools.extension/pyRevit.tab/Project.panel/Preflight Checks.pushbutton/config.py
Outdated
Show resolved
Hide resolved
...sions/pyRevitTools.extension/pyRevit.tab/Project.panel/Preflight Checks.pushbutton/config.py
Outdated
Show resolved
Hide resolved
...sions/pyRevitTools.extension/pyRevit.tab/Project.panel/Preflight Checks.pushbutton/config.py
Outdated
Show resolved
Hide resolved
...sions/pyRevitTools.extension/pyRevit.tab/Project.panel/Preflight Checks.pushbutton/config.py
Outdated
Show resolved
Hide resolved
...sions/pyRevitTools.extension/pyRevit.tab/Project.panel/Preflight Checks.pushbutton/config.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another quick go at the main script.
Since this is a new file, I would start to enforce a bit of formatting rules.
black has good defaults and it is already installed in the pipenv, so you just need to
pipenv run black extensions/pyRevitTools.extension/checks/audit_all_check.py
Co-authored-by: Andrea Ghensi <[email protected]>
Co-authored-by: Andrea Ghensi <[email protected]>
…eflight Checks.pushbutton/config.py Co-authored-by: Andrea Ghensi <[email protected]>
…eflight Checks.pushbutton/config.py Co-authored-by: Andrea Ghensi <[email protected]>
…eflight Checks.pushbutton/config.py Co-authored-by: Andrea Ghensi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments on the query, but this is because it's late...
In general, I have a few points to make:
- why return a list of item and it's count instead of using len(items) where it is needed?
- the
ToElement()
should never be None, at most it is an empty list, so all theis None
checks are useless. Or am I missing something? - use pyrevit.revit.db.query module more 😉 like
get_elements_by_class
... and if there are missing functionality, add it there
Co-authored-by: Andrea Ghensi <[email protected]>
…g FamilyInstance collector
…w filtering logic
Hi @sanzoghenzo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have gone too hard on some things, feel free to pick whatever advice you want 😉
...sions/pyRevitTools.extension/pyRevit.tab/Project.panel/Preflight Checks.pushbutton/config.py
Outdated
Show resolved
Hide resolved
Never mad. This is all constructive criticism. |
I went through the whole thing! Damned... a beast of a refactoring. I learned a lot. Thanks @sanzoghenzo I am ready to merge except for that one SOB groups counting... for the life of me, I spend way too much time on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last one: since there are no None checks in place, avoid having the function arguments default to it
Co-authored-by: Andrea Ghensi <[email protected]>
Co-authored-by: Andrea Ghensi <[email protected]>
Co-authored-by: Andrea Ghensi <[email protected]>
Co-authored-by: Andrea Ghensi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
The puppy is out in the wild. |
📦 New work-in-progress (wip) builds are available for 5.0.0.25031+1700-wip |
Preflight audit of all models, including linked models.
This QC tools returns the following data: