search

sourcebots / robot-api

(Legacy) API to interface with robotd
http://docs.sourcebots.co.uk/api/
MIT License
4 stars 1 forks source link

Fix issues with making socket_path a Path #53

Closed PeterJCLaw closed 6 years ago

PeterJCLaw commented 6 years ago

Following @kierdavis' investigation into the issues with changing the socket_path to being a Path this introduces types in a couple of places where socket_path is used in a non-trivial manner (i.e: not places where we're just passing it through to something else; 6cb4ec7 and aadbaad), renames a variable for clarity (7bdc970) and then introduces a fix (959cf47).

I suggest reviewing by commit -- I've design them so that after reading the tidyups the fix should be obvious and self-explanatory.

kierdavis commented 6 years ago

These changes make sense to me, but since I haven't gotten round to familiarising myself with the typing framework we're using I can't comment on whether these changes are acceptable.

Just to confirm what the fix (959cf47) is doing - presumably the odd behaviour I was seeing is being caused by a Path not being considered equal to a str representing the same filename? This would lead the set difference operation to return new_paths unmodified, since no item of known_paths (which are Paths) would compare equal to any element of new_paths (which are strs), hence breaking the board detection/removal logic.

:+1: for adding typing to the relevant parts of the code here - it makes the problem and its solution much more apparent.

I've copied your source branch to sourcebots/PeterJCLaw--fix-serial-in-base, which allows the CI to automatically run on it. It's complaining about "No module named 'typing'", something I can't offer comment on how to resolve unfortunately.

PeterJCLaw commented 6 years ago

@kierdavis your analysis of the issue is correct.

Do we know why the CI doesn't run for PRs from different repos? (IMO they should be!)

The lack of a typing module is a result of the current default container being a 3.4 one; since the Pis are running Python 3.5 we should change that.

PeterJCLaw commented 6 years ago

The remaining lint failures here appear unrelated to these changes.

RealOrangeOne commented 6 years ago

Do we know why the CI doesn't run for PRs from different repos? (IMO they should be!)

Turns out this is an opt-in feature, for some reason for CircleCI. I've now enabled this for this repo. As the tests do seem to be failing, i'm not sure what we should do about it?

PeterJCLaw commented 6 years ago

@RealOrangeOne the failures on this branch are (I think) lint issues which are more relevant to the underlying serial-in-base branch; I think it's better to fix them there.