Skip to content

Addressed issues 192-194 #196

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

Merged
merged 10 commits into from
Aug 12, 2022
Merged

Conversation

rachlobay
Copy link
Collaborator

Since Issues #192, #193, and #194 are closely related, addressed all three here.

After discussion with Logan, made the following changes to [.epi_df code for the written issue:

Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple a minor things + a request to add any relevant tests for the new checks. Are all the rename-related things already split out into other issues?

@brookslogan
Copy link
Contributor

Also, misc thing: it looks like git might be using attaching your computer username to the commits here, which might be a bit confusing looking at git logs. Could you please try running either:
(1)

git config user.name "<Your Name>"
git config user.email "<email-addr>@<.....>.edu"

in each cloned repo, or
(2)

git config --global user.name "<Your Name>"
git config --global user.email "<your-email-address>@<.....>.edu"

from anywhere?

You can check for success using

git config --list

in each repo, or

git config --global --list

to view just the global settings.

@rachlobay
Copy link
Collaborator Author

Since the rename issues that were mentioned in #192 and #194 were related, I put them into one issue here, where I added a little more info based on our last discussion.

@rachlobay rachlobay requested a review from brookslogan August 11, 2022 05:06
Copy link
Contributor

@dajmcdon dajmcdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once all @brookslogan comments are addressed.

We no longer import anything from vctrs, but do use it in tests; it now belongs
in Suggests: rather than Imports:.
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed a minor check comment. If checks pass, think this is ready to merge.

@rachlobay rachlobay merged commit c5cfb5e into main Aug 12, 2022
@dshemetov dshemetov deleted the 192-to-194-update_epi_df_indexing_fun branch November 15, 2023 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants