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

Transitioning relevant lib/* to app/lib #7649

Closed
wants to merge 6 commits into from
Closed

Conversation

happythenewsad
Copy link
Contributor

@happythenewsad happythenewsad commented May 17, 2021

WHAT

Moves lib/ to app/lib and makes necessary path adjustments for this change.

WHY

@brendanshean 's May 13 eng. meeting discussion
here's a good post for reference

Also, from the zeitwerk author: rails/rails#37835 (comment)
and another relevant comment: rails/rails#13142 (comment)

HOW

Screenshots

(If applicable. Also, please censor any sensitive data)

Notion Card Links

none

PR Checklist Your Answer
Have you added and/or updated tests? yes
Have you deployed to Staging? not yet
Self-Review: Have you done an initial self-review of the code below on Github? yes
Design Review: If applicable, have you compared the coded design to the mockups? (N/A

@happythenewsad happythenewsad temporarily deployed to empirical-grammar-staging May 17, 2021 14:28 Inactive
@brendanshean
Copy link
Member

brendanshean commented May 17, 2021

@happythenewsad Maybe for context in the PR description, here's a good post for reference, specifically, after the UPDATE section.

Also, from the zeitwerk author: rails/rails#37835 (comment)
and another relevant comment: rails/rails#13142 (comment)

@brendanshean
Copy link
Member

I think we should still keep lib/tasks in the repo for rake tasks.

@brendanshean
Copy link
Member

I'd also be curious to hear @dandrabik thoughts on this.

@happythenewsad happythenewsad marked this pull request as draft May 17, 2021 14:59
@happythenewsad
Copy link
Contributor Author

I'm converting this to a draft, because it needs more work. The unit tests are passing but manual testing on staging exposes some uncaught exceptions.

@brendanshean
Copy link
Member

Let me know if there's anything I can help with @happythenewsad . I really appreciate you undertaking this.

@happythenewsad
Copy link
Contributor Author

Let me know if there's anything I can help with @happythenewsad . I really appreciate you undertaking this.

I'm working on sprint work the rest of the day. When one of us grabs this work, let's ping the other so we don't duplicate work. If it's not done by end of sprint I will add it to the backlog.

@dandrabik
Copy link
Member

dandrabik commented May 17, 2021

I'd also be curious to hear @dandrabik thoughts on this.

I'm not opposed to moving items out of lib. It might be worth discussing what the goals of this move would be (since I wasn't there for the meeting). We are autoloading lib, so it could stay there.

I'm assuming some of the goals are to make it clear where to put new files (and to stay within current Rails conventions). We already have a few non-standard app folders in our project, such as app/services, app/queries, etc. and we put some non-ActiveRecord classes in app/models. If we are going to move files, it might make sense to first define where files should go, and then move each file to the appropriate place. This might be more of a reorganization project, than copying over a folder.

Also, as context, like most of those PR comments and that blog post, I typically put code that has no business logic in the lib folder, e.g. third-party API wrappers, utility classes that don't reference models, etc. I would have put many of the files that live in lib now somewhere else.

@brendanshean
Copy link
Member

@dandrabik I think that all sounds reasonable to me.

@brendanshean
Copy link
Member

@happythenewsad I'm going to proceed on this today.

@brendanshean brendanshean changed the title Lib removal Transitioning relevant lib/* to app/lib May 20, 2021
@brendanshean brendanshean marked this pull request as ready for review May 20, 2021 16:18
@happythenewsad
Copy link
Contributor Author

Brendan is continuing this work in a separate PR.

@brendanshean
Copy link
Member

New PR is here: #7678

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.

None yet

3 participants