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

this PR will erroneously fail checks #243

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Feb 15, 2019

No description provided.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 15, 2019

Is this because the user is returned from util.users because you're creating it? I hadn't considered that...

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 15, 2019

Is this because the user is returned from util.users because you're creating it? I hadn't considered that...

Yes. The PR runs first on master and passes, and then on the branch and fails. On the branch this user looks like an existing user.

But the "is this creating an existing user" check isn't needed, because "this PR adds players/foo/bonuses/initial" tells you that it's adding a user.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 15, 2019

That's only true if we never allow anyone into the game without an initial bonus file.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 15, 2019

I think a better check would be that there is no other file under players/<username>

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 15, 2019

I'm going to put a fix into #242 because I think we shouldn't have this allowing dummy users through anyway.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 15, 2019

You could have the PR assert that one of the two is true:

  1. players/username does not exist
  2. players/username contains only a single directory, bonus, which contains a single file, initial

The first will be true on master, the second on the PR branch.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 15, 2019

Yeah, I should have been clearer - we check that there are no files other than initial, and also that initial is being added as part of the PR. The second part is already in the rule, so we just need to add the first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.