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

Add get_user_points #50

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Jan 9, 2019

Add a function to pull points out of the points files, and print them. Still
doesn't make points do anything. Pulled out of #33 to make things easier to
review.

During review of #33 people noted that you might be able to make this function
fail with an Exception saying that you win, which might not be caught earlier
because get_user_points would only be called in determine_if_winner, which
isn't called on PR validation. One way to handle this would be to rewrite the
code defensively, catching every potential issue (something not being a
directory, contents of the file not being an int, etc), but this PR takes a
much simpler (and easier to trust approach) of just additionally running
get_user_points on PR validation. This way, if there's something wrong with
the environment the PR will fail in validation and not be mergeable.

Depends on #47, compatible with #33

Add get_user_points
Add a function to pull points out of the points files, and print them.  Still
doesn't make points do anything.  Pulled out of #33 to make things easier to
review.

During review of #33 people noted that you might be able to make this function
fail with an Exception saying that you win, which might not be caught earlier
because get_user_points would only be called in determine_if_winner, which
isn't called on PR validation.  One way to handle this would be to rewrite the
code defensively, catching every potential issue (something not being a
directory, contents of the file not being an int, etc), but this PR takes a
much simpler (and easier to trust approach) of just additionally running
get_user_points on PR validation.  This way, if there's something wrong with
the environment the PR will fail in validation and not be mergeable.

Depends on #47, compatible with #33
@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 11, 2019

Should the points come out of file names, rather than file contents?

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 11, 2019

No strong feelings, though I guess putting them in the file names would make automated PR analysis simpler.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 11, 2019

I was thinking that it would both be easier and open fewer vulnerabilities, not sure if others agree or care.

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 11, 2019

Contents makes more sense, otherwise ou coul never do duplicate points
and file names explain why

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 11, 2019

@pavellishin

Contents makes more sense, otherwise you could never do duplicate points and file names explain why

I'm imagining something like:

players/pavellishin/bonuses/initial_10
players/pavellishin/bonuses/reject_pr_100-1
players/pavellishin/bonuses/bribe_from_tnelling_4

That is:

  • file name must contain at least one underscore
  • the final component after the underscore must match [-0-9]+, that is, the digits and the minus sign.
  • the file is empty

But I do agree that file names like:

players/pavellishin/bonuses/10
players/pavellishin/bonuses/-1
players/pavellishin/bonuses/4

would not work.

jeffkaufman added a commit that referenced this pull request Jan 11, 2019

named-bonuses: alternative implementation
This is an alternative to #47, where the point value is in the filename instead of the file body.  Suggested in the comments on #50, the idea is this makes it easier to parse diffs that work with points.

I also added two example files to my directory, one showing point gain and one showing point loss, so you can see what negative numbers look like.

I don't have strong feelings between this and #47, and would be happy with either.
@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 11, 2019

This currently works with #47 (points in file body), but I'm happy to rewrite it to go with #57 (points in file name) if we go that way instead.

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 17, 2019

@jeffkaufman conflict!

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 17, 2019

hold off on reviewing this until #47 goes in

@jeffkaufman jeffkaufman added the blocked label Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment