-
Notifications
You must be signed in to change notification settings - Fork 125
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
west: Add untracked files and folders to west status output #702
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -844,6 +844,53 @@ def do_run(self, args, user_args): | |||||
failed.append(project) | ||||||
self._handle_failed(args, failed) | ||||||
|
||||||
# List projects not tracked by manifest. | ||||||
projects = [project.abspath for project in self._cloned_projects(args, only_active=True)] | ||||||
topdir = Path(self.topdir) | ||||||
untracked = [] | ||||||
|
||||||
self.do_find_untracked(topdir, projects, untracked) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Non-obvious mix of "OUT" and "IN" parameters is very confusing. |
||||||
|
||||||
ignored_config = self.config.get('status.ignore') | ||||||
ignored = ignored_config.split(',') if ignored_config else [] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a separate |
||||||
# Ignore .west folder in untracked projects | ||||||
ignored.append('.west/') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would anyone have a |
||||||
for ignore in ignored: | ||||||
try: | ||||||
untracked.remove(ignore) | ||||||
except: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does something this mundane really need a |
||||||
pass | ||||||
|
||||||
if len(untracked) > 0: | ||||||
self.banner('untracked:') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Please do not re-use the same presentation as git to avoid misleading users into thinking this line comes from git. |
||||||
for project in untracked: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
You seem to assume that anything in topdir is either an active or inactive "project". That's not true for everyone, in fact we (SOF) use topdir as a very convenient place to place build directories - very convenient because it does not require any |
||||||
self.inf(textwrap.indent(f'{project}', ' ' * 4)) | ||||||
|
||||||
def is_manifest_subfolder(self, path, projects): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either I don't understand this function, or the name is very confusing. Do you mean something like The manifest is a list of git projects, it's not the projects themselves. Anyway you should not need such a path comparison because the directory walk should prune |
||||||
for project_path in projects: | ||||||
try: | ||||||
if Path(project_path).relative_to(Path(path)): | ||||||
return True | ||||||
except ValueError: | ||||||
pass | ||||||
return False | ||||||
|
||||||
def do_find_untracked(self, path, projects, untracked): | ||||||
obj = os.scandir(path) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is a list, use plural. Also, stick to the standard Python terminology when possible. |
||||||
|
||||||
for entry in obj: | ||||||
if entry.is_dir(): | ||||||
if entry.path in projects: | ||||||
continue | ||||||
if self.is_manifest_subfolder(entry.path, projects): | ||||||
self.do_find_untracked(entry, projects, untracked) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think If not, add a comment explaining why you can't use os.walk() and have to perform the recursion yourself. |
||||||
else: | ||||||
untracked.append(os.path.relpath(entry.path, Path(self.topdir)) + '/') | ||||||
if entry.is_file(): | ||||||
untracked.append(os.path.relpath(entry.path, Path(self.topdir))) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're using a mix or Generally speaking this what this code does is not always very obvious and there's zero zero comment right now. |
||||||
|
||||||
obj.close() | ||||||
|
||||||
class Update(_ProjectCommand): | ||||||
|
||||||
def __init__(self): | ||||||
|
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.
One of the purposes is to make a difference between active an inactive projects but this first line does not.