Skip to content
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

Closed
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions src/west/app/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
projects = [project.abspath for project in self._cloned_projects(args, only_active=True)]
active_projects = [project.abspath for project in self._cloned_projects(args, only_active=True)]

One of the purposes is to make a difference between active an inactive projects but this first line does not.

topdir = Path(self.topdir)
untracked = []

self.do_find_untracked(topdir, projects, untracked)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.do_find_untracked(topdir, projects, untracked)
untracked = self.do_find_untracked(topdir, active_projects, projects)

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 []
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a separate .westignore file for this for consistency with git and others. This will let users put each pattern on a separate line and let them ignore files with commas in their names.

# Ignore .west folder in untracked projects
ignored.append('.west/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would anyone have a .west/ folder in untracked projects? This looks like a problem most people would want to see (while others can explicitly ignore it)

for ignore in ignored:
try:
untracked.remove(ignore)
except:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does something this mundane really need a try/except? Dubious.

pass

if len(untracked) > 0:
self.banner('untracked:')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.banner('untracked:')
self.banner('Unknown:')

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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for project in untracked:
for entry in untracked:

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 .gitignore-like file (yet!)

self.inf(textwrap.indent(f'{project}', ' ' * 4))

def is_manifest_subfolder(self, path, projects):
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 is_in_some_west_project()?

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 west projects as soon as it finds one?

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
obj = os.scandir(path)
entries = os.scandir(path)

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think os.walk() does this recursion for you:
https://stackoverflow.com/questions/19859840/excluding-directories-in-os-walk

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)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're using a mix or Path.relative_to() and os.path.relpath(). If there's a reason why then add a short comment explaining why, otherwise stick to Path.

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):
Expand Down
Loading