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

remove FIXME and (cosmicexplorer) comments #6479

Merged
merged 3 commits into from Sep 16, 2018

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Sep 10, 2018

Problem

  • FIXME is a bit judgy for no well-defined additional meaning beyond TODO.
  • The TODO(name) syntax is (mostly) deprecated / all of the instances of its use here have enough context on their own.
@Eric-Arellano
Copy link
Contributor

Eric-Arellano left a comment

I really like this change! I remember being confused the first time I saw FIXME, and TODO gives off a much better and welcoming tone.

What’s the reason to remove the names from TODO? The benefit of names is it makes it clear to others they don’t need to spend time on fixing it, because the person who annotated it can probably solve it much more quickly.

@@ -217,8 +217,6 @@ def test_blows_up_on_invalid_args(self):
with self.assertRaises(TypeCheckError):
self._default_args_execute_process_request(argv=('1',), env=['foo', 'bar'])

# TODO(cosmicexplorer): we should probably check that the digest info in
# ExecuteProcessRequest is valid, beyond just checking if it's a string.
with self.assertRaisesRegexp(TypeCheckError, "env"):

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Sep 10, 2018

Contributor

TODO no longer relevant?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 10, 2018

Contributor

Yes, the ExecuteProcessRequest constructor does some type checking/coercion now.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Sep 10, 2018

Leaving a name alone isn't much context -- if there's more context than can fit in a short enough comment, an issue should be made instead (I use TODO(#1234) for that), if not you risk leaving that context in your head. This is mostly because I am very bad at being able to keep that context in my head, personally speaking.

@Eric-Arellano
Copy link
Contributor

Eric-Arellano left a comment

Sounds good, particularly because we seem to prefer the issue number system instead (at Foursquare we used name system).

It may be helpful for us to start requesting PRs to open an issue anytime they add a TODO. Stu has requested this of me in the past, and would be good for us to all do the same.

Thanks again for this change!

@stuhood
Copy link
Member

stuhood left a comment

Fine with this, but a few of these read like rhetorical questions ("Do we need to test this?") and deleting them would be preferable.

cosmicexplorer added some commits Sep 5, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-todo-fixme branch from f30b57e to 46fd3ac Sep 16, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Sep 16, 2018

@stuhood in 46fd3ac I removed most of the rhetorical questions -- one remains, because it's getting removed in #6374.

@cosmicexplorer cosmicexplorer merged commit 606fc83 into pantsbuild:master Sep 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment