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

Hash lookup for Inat::Obs attributes #2418

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Conversation

JoeCohen
Copy link
Member

@JoeCohen JoeCohen commented Sep 25, 2024

  • Uses hash lookup to access Inat::Obs attributes.
  • Removes needless, repetitive pattern of defined methods.
    Ex: inat_obs[:id] instead of inat_obs.inat_id
  • Fixes Inat::Obs class length metric offense.
  • Responds to @mo-nathan suggestion.
  • Replaces Inat::Obs class length #2416

There is no manual test.

- fixes all but taxon-related methods
@JoeCohen JoeCohen self-assigned this Sep 25, 2024
@JoeCohen JoeCohen added iNat interaction with iNat refactor labels Sep 25, 2024
@coveralls
Copy link
Collaborator

coveralls commented Sep 25, 2024

Coverage Status

coverage: 93.453% (-0.003%) from 93.456%
when pulling fe066d5 on jdc-inat_obs-hash-lookup
into 8c9c794 on main.

@JoeCohen JoeCohen mentioned this pull request Sep 25, 2024
@JoeCohen JoeCohen marked this pull request as ready for review September 25, 2024 17:14
@nimmolo
Copy link
Contributor

nimmolo commented Sep 28, 2024

Joe - I believe there is a better, more standard Railsy way to do all this, and there's an example on my PR:

Define your inat_obs as an ActiveModel::Model object, rather than a bare PORO.

This gives you the accessor methods without repetitive declarations, and potentially other goodies besides, like validation (although i didn't use that). You won't have to access your attributes by hash key. It makes it work like a model instance (because it is).

It's a "model not backed by a db table" and it's a very convenient thing. This is what people do for form data where the are assembling attributes that may go into some other model (like an observation, in your case) but they want the attributes of the form accessible like a model, too.

I've known about this for years but never understand quite how to use it, but now that I did, I realize how easy it is and will be using it elsewhere.

Check ObservationFilter at app/models/observation_filter.rb and NameFilter, which are based on a model SearchFilter that inherits from ActiveModel. It's at app/models/search_filter.rb.

class SearchFilter
  include ActiveModel::Model
  include ActiveModel::Attributes

  attribute :pattern, :string

@JoeCohen
Copy link
Member Author

JoeCohen commented Sep 28, 2024 via email

@nimmolo
Copy link
Contributor

nimmolo commented Sep 28, 2024

#2337 — which as of yesterday does not require any migrations to check out, so it won't mess up your local db!

@JoeCohen JoeCohen added this to the Basic iNat Import milestone Sep 29, 2024
@JoeCohen
Copy link
Member Author

JoeCohen commented Sep 30, 2024

@nimmolo: TLDR: I like the idea of a model not backed by a db table, but will postpone it till after NEMF.

  1. I want a good window, before NEMF, for live shakedown tests of iNat import.
  2. What I have now works, so I'd prefer to postpone refactors until after NEMF.
  3. The hash stuff is convenient because it differentiates the raw iNat data from methods which manipulate or map that data. But that might indicate that Inat::Obs has too many concerns, and therefore that I should create another class. That's something that I certainly don't want to attempt prior to NEMF.
  4. The hash stuff also works with all levels of the iNat data. I don't know about a model not backed by a db table, and don't want to take time right now find an answer.

Joe - I believe there is a better, more standard Railsy way to do all this, and there's an example on my PR:

Define your inat_obs as an ActiveModel::Model object, rather than a bare PORO.

This gives you the accessor methods without repetitive declarations, and potentially other goodies besides, like validation (although i didn't use that). You won't have to access your attributes by hash key. It makes it work like a model instance (because it is).

It's a "model not backed by a db table" and it's a very convenient thing. This is what people do for form data where the are assembling attributes that may go into some other model (like an observation, in your case) but they want the attributes of the form accessible like a model, too.

I've known about this for years but never understand quite how to use it, but now that I did, I realize how easy it is and will be using it elsewhere.

Check ObservationFilter at app/models/observation_filter.rb and NameFilter, which are based on a model SearchFilter that inherits from ActiveModel. It's at app/models/search_filter.rb.

class SearchFilter
  include ActiveModel::Model
  include ActiveModel::Attributes

  attribute :pattern, :string

@JoeCohen JoeCohen merged commit ad31c4e into main Sep 30, 2024
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iNat interaction with iNat refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants