Skip to content

Conversation

@jonavellecuerdo
Copy link
Contributor

@jonavellecuerdo jonavellecuerdo commented Sep 5, 2023

What does this PR do?

Refine code naming conventions and in-code documentation to improve code coherency and repo
structure.

How can a reviewer manually see the effects of these changes?

Review PR and examine if added/updated docstrings and code naming conventions are informative
or helpful in explaining the app's workflow.

Includes new or updated dependencies?

NO

Developer

  • README is updated to reflect all changes as needed
  • All related Jira tickets are linked in commit message(s)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated to reflect all changes or is not needed
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Why these changes are being introduced:
* Improve code coherency and repo structure

How this addresses that need:
* Rename FTPFeed class to DatabaseToFtpPipe
* Rename FTPReader to FtpFileWriter
* Rename PipeWriter.pipe to PipeWriter.connect
* Reorganize app module so classes appear after methods
* Add docstrings to cli and app modules

Side effects of this change:
* None

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/IN-905
@jonavellecuerdo jonavellecuerdo self-assigned this Sep 5, 2023
@ehanson8 ehanson8 self-requested a review September 5, 2023 17:32
Copy link
Contributor

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

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

A lot of comments but this is all looking great, let's have a quick chat tomorrow about a few things I noticed in the modules not covered by this PR

* Add and update docstrings
* Simplify PipeWriter class
* Additional updates to variable and class names
* Move transform functions to helpers.py
@jonavellecuerdo jonavellecuerdo force-pushed the IN-905-refine-var-names-and-docstrings branch 3 times, most recently from ce57328 to 6f2f943 Compare September 6, 2023 15:41
@jonavellecuerdo
Copy link
Contributor Author

@ehanson8 Please see the latest commit! For minor comments, I marked them as 'resolved' as I worked through the changes, but I left the open-ended ones that still need you review. Let me know what you think. :)

Copy link
Contributor

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

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

Fantastic work! 1 possible typo

@jonavellecuerdo jonavellecuerdo force-pushed the IN-905-refine-var-names-and-docstrings branch from 6f2f943 to 5c63810 Compare September 6, 2023 18:53
@jonavellecuerdo jonavellecuerdo merged commit 566509e into main Sep 6, 2023
@jonavellecuerdo jonavellecuerdo deleted the IN-905-refine-var-names-and-docstrings branch September 6, 2023 18:58
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.

2 participants