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

Rename DF functions to be consistent with Python DF functions. #379

Merged
merged 1 commit into from
Nov 17, 2019

Conversation

ruebot
Copy link
Member

@ruebot ruebot commented Nov 14, 2019

GitHub issue(s): #366

What does this Pull Request do?

Rename DF functions to be consistent with Python DF functions.

How should this be tested?

  • TravisCI (unit tests) should take care of it.

Additional Notes:

  1. @lintool the only part of the issue I haven't resolved here is your proposal to move WebArchive to RecordLoader.loadArchives on the Python side. I don't believe we can have RecordLoader.loadArchives as an actual class name. So, should we change things in the other direction or both directions? I've been naming a val warcs a lot in my example. What if we did something like that? Or webArchives or webarchives ? Once we settle on something, I'll the PR out of draft.

  2. There will be a TONNE of documentation updates to do. If I could get a hand there, that'd be super helpful. @SamFritz @ianmilligan1 that of any interest?

@ruebot ruebot requested a review from lintool November 14, 2019 23:04
@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #379 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #379   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files          40       40           
  Lines        1418     1418           
  Branches      268      268           
=======================================
  Hits         1080     1080           
  Misses        221      221           
  Partials      117      117

@ianmilligan1
Copy link
Member

👀

Looks like a great PR in progress, and agreed, the documentation will be heavy-ish lifting. I can help out if you give me discrete chunks of work (between parental leave, grant writing, admin, and some unfortunate university admin work coming up, my time is a bit limited, but many hands makes for light work).

@lintool
Copy link
Member

lintool commented Nov 15, 2019

@rubot I'm not wedded to perfect alignment between Scala DF and Pyspark DF at this point... perhaps we should just wait for #371?

I would rather do the refactoring incrementally... i.e., let's get this merged, start fixing docs, etc.

@ruebot
Copy link
Member Author

ruebot commented Nov 15, 2019

@lintool cool 🤘 🤘

@ruebot ruebot marked this pull request as ready for review November 15, 2019 23:36
@ianmilligan1 ianmilligan1 merged commit 67ca17d into master Nov 17, 2019
@ianmilligan1 ianmilligan1 deleted the issue-366 branch November 17, 2019 02:26
@ianmilligan1
Copy link
Member

Great! @ruebot and I can maybe work together or the docs? When I have time I can start a branch or vice versa.

@ruebot
Copy link
Member Author

ruebot commented Nov 17, 2019

Cool. I'll update archivesunleashed/aut-docs#21 with the new names, then we can go from there? Or, if you want to start on another file, go for it. Shouldn't be too much besides that hopefully.

@ianmilligan1
Copy link
Member

Sure, sounds good on updating the PR and going fro, there! I might be able to tackle some of this tomorrow, kids schedule permitting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants