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

Suggestion: ungroup the result from get_afltables_stats() #38

Closed
neilfws opened this issue Sep 5, 2018 · 1 comment
Closed

Suggestion: ungroup the result from get_afltables_stats() #38

neilfws opened this issue Sep 5, 2018 · 1 comment
Assignees

Comments

@neilfws
Copy link

@neilfws neilfws commented Sep 5, 2018

get_afltables_stats() returns a tibble grouped by Season, Round, Home.team, Away.team.

I would suggest that the function should include ungroup() as the last step before returning results. It isn't clear to the end user that the output is grouped unless they inspect the tibble, and grouping may have unintended consequences in any code that the user writes which uses the tibble.

@jimmyday12 jimmyday12 self-assigned this Sep 5, 2018
@jimmyday12
Copy link
Owner

@jimmyday12 jimmyday12 commented Sep 5, 2018

Thanks @neilfws, I agree. There are a few inconsistencies like that I'd like to fix up. Open to PR if you are interested, otherwise I'll take a look over the next week and keep you posted.

@jimmyday12 jimmyday12 closed this in e8be939 Oct 1, 2018
jimmyday12 added a commit that referenced this issue Oct 1, 2018
fixes #38 output of get_afltables_stats() is no longer grouped
jimmyday12 added a commit that referenced this issue Oct 1, 2018
Merge branch 'master' into develop

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.