DRY up api/errors and api2/errors?

[nimmolo]

I noticed the obvious duplication between the API and API2 classes, and I understand that's intentional, to keep the first API working while making changes on the second.

First question, is there a way to DRY these? Could we have each subclass of API2 inherit from the analagous subclass of API, then just overwrite methods that are changing? I have no idea if that would work, but it would also serve as a record of changes in the API.

Secondly and separately, the Error subclass of each of them is also nearly identical, and has many classes that inherit from it. I was wondering about the wisdom of making a separate APIError class, with subclasses like NoMethodForAction < APIError. The consideration here is that Zeitwerk enforces one class per file, so it might be nice to namespace all these errors into APIError::NoMethodForAction < APIError and put them in their own directory under classes/api_error.

Background:
As i'm playing around on the Zeitwerk branch, I'm finding there's a balance to be struck between minimizing refactoring, and minimizing the number of new exceptions and requires to tell Zeitwerk about the quirks in our directory structure. I suspect writing a bunch of exceptions and explicit requires will likely create headaches going forward, so i'm looking to conventionalize and DRY up a few things.

As everyone probably knows, Zeitwerk is a new autoload engine based on a kind of "dumber is faster" approach. By assuming stricter correspondence between directories and class naming, they say it can work more efficiently. It's opt-in on Rails 6, so we can get used to the water, and mandatory on Rails 7.

[nimmolo]

I guess it would be more like

module APIError
  class Base
  end
end

module APIError
  class HelpMessage < Base
  end
end

[pellaea]

Well, you're allowed to do whatever you like, of course, but I would still strongly recommend against mixing API and API2 in any way. I don't consider API to be a maintenance liability since we can retire it whenever we like, and in the meantime *by definition* we aren't allowed to touch it so it can't incur any maintenance liability. Now if you want to refactor API2, knock yourself out. But again, please try to make sure it produces identical results. Otherwise it could potentially break all sorts of code external to MO and we have no way of knowing who is using it or how. Nor do we even have any way to contact users to warn them of a change.

…

On Fri, Jun 3, 2022 at 4:17 PM andrew nimmo ***@***.***> wrote: I guess it would be more like module APIError class Base end end module APIError class HelpMessage < Base end end — Reply to this email directly, view it on GitHub <#982 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNOJ3Y4VYEXALSONDJ3VNJR7LANCNFSM5XZ4D3FA> . You are receiving this because you are subscribed to this thread.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2880622@ github.com>

[nimmolo]

Got it. I will therefore not share APIError between the classes; they can stay API::Error and API2::Error.

Producing identical results is certainly the goal. How do you feel about the API tests? They seem pretty exhaustive, but you'd know better.

The issue i'm trying to solve regards making Zeitwerk sturdy and low maintenance. Not refactoring the API class and its directories means writing exceptions for Zeitwerk, to get that class to load properly under Zeitwerk. Refactoring the two class directories both similarly would keep the config simpler. I feel like if the tests pass in both cases, it should be kosher.

Incidentally, re: contacting users, i assume we tried emailing users with an API key to ask them to switch.

[pellaea]

I don't see a problem with an ugly config for zeitwerk to grandfather in API. Doesn't matter how ugly that code is. Again, feel free to do whatever you want to API2 to make it work better with zeitwerk. Tests for API (or API2) are definitely not thorough, not by a long shot. Most users of the API don't have an API key. I tried to look at the logs once upon a time to see who was using it, and it was a mess of IPs I couldn't connect with accounts. And it didn't look like robots. Maybe it's all different now. And maybe the next version of the API (API3?) we should require some sort of identification precisely to solve this problem. But we didn't, so we have the problem. Of course, as always, just my opinion.

…

On Fri, Jun 3, 2022 at 5:57 PM andrew nimmo ***@***.***> wrote: Got it. I will therefore not share APIError between the classes; they can stay API::Error and API2::Error. Producing identical results is certainly the goal. How do you feel about the API tests? They seem pretty exhaustive, but you'd know better. The issue i'm trying to solve regards making Zeitwerk sturdy and low maintenance. *Not* refactoring the API class and its directories means writing exceptions for Zeitwerk, to get that class to load properly under Zeitwerk. Refactoring the two class directories both similarly would keep the config simpler. I feel like if the tests pass in both cases, it should be kosher. Incidentally, re: contacting users, i assume we tried emailing users with an API key to ask them to switch. — Reply to this email directly, view it on GitHub <#982 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNMEXK6WQ7TJWRKBRMTVNJ5TNANCNFSM5XZ4D3FA> . You are receiving this because you commented.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881001@ github.com>

[nimmolo]

This is all good to know.

[JoeCohen]

Jason: Do you remember how (if at all) we notified people about the new API? On Fri, Jun 3, 2022 at 3:04 PM Jason Hollinger ***@***.***> wrote:

…

I don't see a problem with an ugly config for zeitwerk to grandfather in API. Doesn't matter how ugly that code is. Again, feel free to do whatever you want to API2 to make it work better with zeitwerk. Tests for API (or API2) are definitely not thorough, not by a long shot. Most users of the API don't have an API key. I tried to look at the logs once upon a time to see who was using it, and it was a mess of IPs I couldn't connect with accounts. And it didn't look like robots. Maybe it's all different now. And maybe the next version of the API (API3?) we should require some sort of identification precisely to solve this problem. But we didn't, so we have the problem. Of course, as always, just my opinion. On Fri, Jun 3, 2022 at 5:57 PM andrew nimmo ***@***.***> wrote: > Got it. I will therefore not share APIError between the classes; they can > stay API::Error and API2::Error. > > Producing identical results is certainly the goal. How do you feel about > the API tests? They seem pretty exhaustive, but you'd know better. > > The issue i'm trying to solve regards making Zeitwerk sturdy and low > maintenance. *Not* refactoring the API class and its directories means > writing exceptions for Zeitwerk, to get that class to load properly under > Zeitwerk. Refactoring the two class directories both similarly would keep > the config simpler. I feel like if the tests pass in both cases, it should > be kosher. > > Incidentally, re: contacting users, i assume we tried emailing users with > an API key to ask them to switch. > > — > Reply to this email directly, view it on GitHub > < #982 (reply in thread) >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAYTNNMEXK6WQ7TJWRKBRMTVNJ5TNANCNFSM5XZ4D3FA > > . > You are receiving this because you commented.Message ID: > <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881001@ > github.com> > — Reply to this email directly, view it on GitHub <#982 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALDFEILFJKPKGOIWZBVADVNJ6QBANCNFSM5XZ4D3FA> . You are receiving this because you are subscribed to this thread.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881026@ github.com>

[pellaea]

I don't think we did. Maybe a site banner? On Fri, Jun 3, 2022 at 6:16 PM Joseph D. Cohen ***@***.***> wrote:

…

Jason: Do you remember how (if at all) we notified people about the new API? On Fri, Jun 3, 2022 at 3:04 PM Jason Hollinger ***@***.***> wrote: > I don't see a problem with an ugly config for zeitwerk to grandfather in > API. Doesn't matter how ugly that code is. > > Again, feel free to do whatever you want to API2 to make it work better > with zeitwerk. > > Tests for API (or API2) are definitely not thorough, not by a long shot. > > Most users of the API don't have an API key. I tried to look at the logs > once upon a time to see who was using it, and it was a mess of IPs I > couldn't connect with accounts. And it didn't look like robots. Maybe > it's all different now. And maybe the next version of the API (API3?) we > should require some sort of identification precisely to solve this > problem. But we didn't, so we have the problem. > > Of course, as always, just my opinion. > > On Fri, Jun 3, 2022 at 5:57 PM andrew nimmo ***@***.***> > wrote: > > > Got it. I will therefore not share APIError between the classes; they can > > stay API::Error and API2::Error. > > > > Producing identical results is certainly the goal. How do you feel about > > the API tests? They seem pretty exhaustive, but you'd know better. > > > > The issue i'm trying to solve regards making Zeitwerk sturdy and low > > maintenance. *Not* refactoring the API class and its directories means > > writing exceptions for Zeitwerk, to get that class to load properly under > > Zeitwerk. Refactoring the two class directories both similarly would keep > > the config simpler. I feel like if the tests pass in both cases, it > should > > be kosher. > > > > Incidentally, re: contacting users, i assume we tried emailing users with > > an API key to ask them to switch. > > > > — > > Reply to this email directly, view it on GitHub > > < > #982 (reply in thread) > >, > > or unsubscribe > > < > https://github.com/notifications/unsubscribe-auth/AAYTNNMEXK6WQ7TJWRKBRMTVNJ5TNANCNFSM5XZ4D3FA > > > > . > > You are receiving this because you commented.Message ID: > > > <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881001@ > > github.com> > > > > — > Reply to this email directly, view it on GitHub > < #982 (comment) >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAALDFEILFJKPKGOIWZBVADVNJ6QBANCNFSM5XZ4D3FA > > . > You are receiving this because you are subscribed to this thread.Message > ID: > <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881026@ > github.com> > — Reply to this email directly, view it on GitHub <#982 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNLTSO4ZHGBAXRAF6ZLVNJ73PANCNFSM5XZ4D3FA> . You are receiving this because you commented.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881064@ github.com>

[pellaea]

I would say very firmly, NO. I request that you leave API completely alone. Any improvements you want to make, make to API2. But make sure any development to API2 does not change its functionality at all (outside of adding more fields and/or requests -- add, don't subtract or change!) If you must make a nontrivial change to its functionality, we need to clone it again into API3. But obviously we're resisting that.

…

On Fri, Jun 3, 2022 at 3:02 PM andrew nimmo ***@***.***> wrote: I noticed the obvious duplication between the API and API2 classes, and I understand that's intentional, to keep the first API working while making changes on the second. First, is there a way to DRY these? Could we have each subclass of API2 inherit from the analagous subclass of API, then just overwrite methods that are changing? I have no idea if that would work, but it would also serve as a record of changes in the API. Secondly and separately, the Error subclass of each of them is also nearly identical, and has many classes that inherit from it. I was wondering about the wisdom of making a separate APIError class, with subclasses like NoMethodForAction < APIError. The consideration here is that Zeitwerk enforces one class per file, so it might be nice to namespace all these errors into APIError::NoMethodForAction < APIError and put them in their own directory under classes/api_error. *Background*: As i'm playing around on the Zeitwerk branch, I'm finding there's a balance to be struck between *minimizing refactoring*, and minimizing the number of new exceptions and requires_ to tell Zeitwerk about the quirks in our directory structure. I suspect writing a bunch of exceptions and explicit requires will likely create headaches going forward, so i'm looking to conventionalize and DRY up a few things. As everyone probably knows, Zeitwerk is a new autoload engine based on a kind of "dumber is faster" approach. By assuming stricter correspondence between directories and class naming, they say it can work more efficiently. It's opt-in on Rails 6, so we can get used to the water, and mandatory on Rails 7. — Reply to this email directly, view it on GitHub <#982>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNPC2L2GXB2DKV5ZR2DVNJJFBANCNFSM5XZ4D3FA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[JoeCohen]

We could try what Andrew suggested. Do you think it's worth the trouble? 2.7.6 :005 > User.joins(:api_keys).uniq.count => 206 I could email them in small batches from the webmaster gmail account. On Fri, Jun 3, 2022 at 3:25 PM Jason Hollinger ***@***.***> wrote:

…

I don't think we did. Maybe a site banner? On Fri, Jun 3, 2022 at 6:16 PM Joseph D. Cohen ***@***.***> wrote: > Jason: > Do you remember how (if at all) we notified people about the new API? > > > On Fri, Jun 3, 2022 at 3:04 PM Jason Hollinger ***@***.***> > wrote: > > > I don't see a problem with an ugly config for zeitwerk to grandfather in > > API. Doesn't matter how ugly that code is. > > > > Again, feel free to do whatever you want to API2 to make it work better > > with zeitwerk. > > > > Tests for API (or API2) are definitely not thorough, not by a long shot. > > > > Most users of the API don't have an API key. I tried to look at the logs > > once upon a time to see who was using it, and it was a mess of IPs I > > couldn't connect with accounts. And it didn't look like robots. Maybe > > it's all different now. And maybe the next version of the API (API3?) we > > should require some sort of identification precisely to solve this > > problem. But we didn't, so we have the problem. > > > > Of course, as always, just my opinion. > > > > On Fri, Jun 3, 2022 at 5:57 PM andrew nimmo ***@***.***> > > wrote: > > > > > Got it. I will therefore not share APIError between the classes; they > can > > > stay API::Error and API2::Error. > > > > > > Producing identical results is certainly the goal. How do you feel > about > > > the API tests? They seem pretty exhaustive, but you'd know better. > > > > > > The issue i'm trying to solve regards making Zeitwerk sturdy and low > > > maintenance. *Not* refactoring the API class and its directories means > > > writing exceptions for Zeitwerk, to get that class to load properly > under > > > Zeitwerk. Refactoring the two class directories both similarly would > keep > > > the config simpler. I feel like if the tests pass in both cases, it > > should > > > be kosher. > > > > > > Incidentally, re: contacting users, i assume we tried emailing users > with > > > an API key to ask them to switch. > > > > > > — > > > Reply to this email directly, view it on GitHub > > > < > > > #982 (reply in thread) > > >, > > > or unsubscribe > > > < > > > https://github.com/notifications/unsubscribe-auth/AAYTNNMEXK6WQ7TJWRKBRMTVNJ5TNANCNFSM5XZ4D3FA > > > > > > . > > > You are receiving this because you commented.Message ID: > > > > > > <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881001@ > > > github.com> > > > > > > > — > > Reply to this email directly, view it on GitHub > > < > #982 (comment) > >, > > or unsubscribe > > < > https://github.com/notifications/unsubscribe-auth/AAALDFEILFJKPKGOIWZBVADVNJ6QBANCNFSM5XZ4D3FA > > > > . > > You are receiving this because you are subscribed to this thread.Message > > ID: > > > <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881026@ > > github.com> > > > > — > Reply to this email directly, view it on GitHub > < #982 (comment) >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAYTNNLTSO4ZHGBAXRAF6ZLVNJ73PANCNFSM5XZ4D3FA > > . > You are receiving this because you commented.Message ID: > <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881064@ > github.com> > — Reply to this email directly, view it on GitHub <#982 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALDFAKUNAG2BJIASOJLH3VNKA55ANCNFSM5XZ4D3FA> . You are receiving this because you commented.Message ID: <MushroomObserver/mushroom-observer/repo-discussions/982/comments/2881107@ github.com>