search

opencaching / opencaching-pl

The source code of Opencaching.PL (and some other domains)
https://opencaching.pl/
GNU General Public License v3.0
22 stars 33 forks source link

Refactoring of geocache attributes #2304

Closed kojoty closed 2 years ago

kojoty commented 2 years ago

HI,

This is general refactoring of attributes for geocaches. I would like to clean it up, unify, move definition of attributes (config) from DB to files etc.

This is ONLY preparation - after merging this everything works the same - this code is not used at all.

How it works:

Still to do:

I'm going to merge it soon, as this not affects the working code. If we decide it's OK I will deliver the changes to use the new approach in views with attributes. Next after delivering OKAPI changes we will be able to finally remove cache_attrib table from DB :)

andrixnet commented 2 years ago

This is great news. I've been working on attribute definitions the previous years (dataset) a lot and also created an attribute set (graphics). I paused because of the code changes it required. I am happy to share my work.

Please include attribute theme configuration directive.

Also note that OC UK has requested at least one new attribute (I had extensive talks with them in 2019) and the removal of several. I'll look it up and post it.

My work also included opencaching.eu documentation and a partial redefining of the attribute IDs from the current mess to a consistent, cross node uniform set.

IIRC I developed some SQL queries to help make the transition for cache - attribute mappings.

Don't forget OC US has some particular attribs as well, since they dropped some cache types in favour of attributes.

andrixnet commented 2 years ago

I'll recheck my attribute sheets and update the wiki.

andrixnet commented 2 years ago

Checking https://wiki.opencaching.eu/index.php?title=Cache_attributes against my master table. I think I have to update it with several of the OCUS attribs (derived from cache type fallback).

Also, OCUK would like to add at least new attrib 217.

Before applying SQL updates on data (per note designed SQL queries) I think it best that I recheck them. (also in light with OCUS changes) See email in that regard.

andrixnet commented 2 years ago

Reading the code looks like I should make additional updates to the wiki (and my tables). Just to bring things in sync. Thank you very much for tackling this very large component.

andrixnet commented 2 years ago

Issues reference: https://github.com/opencaching/opencaching-pl/issues/462 https://github.com/opencaching/opencaching-pl/issues/806 https://github.com/opencaching/opencaching-pl/issues/1251 https://github.com/opencaching/opencaching-pl/issues/2013 https://github.com/opencaching/opencaching-pl/issues/2076 https://github.com/opencaching/opencaching-pl/issues/2277 at least.

deg-pl commented 2 years ago

From Facebook UK technical group:

I've had a look and it seems there are just 3 questions for us ... OCUK: ASKOWNER [id=156] - what is this? why do we need this? I have no idea ! OCUK: OTHER [id=157] - what is this? why do we need this? I have no idea on this one either OCUK: AIRCRAFT [id=153] - really? how many such caches are there? It seems unlikely there are any in the UK I have no problem with dropping all of these.

@kojoty could you check if any OC UK caches has these attributes?

kojoty commented 2 years ago

OK I deliver the current version of the code and we will work on this. For now this is harmless as this code is not used anywhere.

kojoty commented 2 years ago

BTW. Please see https://opencaching.pl/test/attributes for debug current config debug purpose :)

kojoty commented 2 years ago

From Facebook UK technical group:

I've had a look and it seems there are just 3 questions for us ... OCUK: ASKOWNER [id=156] - what is this? why do we need this? I have no idea ! OCUK: OTHER [id=157] - what is this? why do we need this? I have no idea on this one either OCUK: AIRCRAFT [id=153] - really? how many such caches are there? It seems unlikely there are any in the UK I have no problem with dropping all of these.

@kojoty could you check if any OC UK caches has these attributes?

There is no Aircraft nor AskOwner, 2 caches with attrib. OTHER.

kojoty commented 2 years ago

no php8 to w ogole wiele nowych uproszczeń w składni - no ale jeszcze chwilę poczekajmy...

On Tue, Oct 19, 2021 at 11:15 AM Grzegorz Deja - deg < @.***> wrote:

@.**** commented on this pull request.

In src/Models/GeoCache/CacheAttribute.php https://github.com/opencaching/opencaching-pl/pull/2304#discussion_r731658861 :

    • Each node support only subset of attributes presents below.

+class CacheAttribute

+{

+

  • // attribute name = attribute ID

  • /* Access or parking fee /

  • const FEE = 2;

  • /* Climbing gear requried /

  • const RAPPELING = 3;

  • /* Boat required /

  • const BOAT = 4;

Alternatywnie można byłoby wczytywać te stałe w postaci tablic i przerabiać je pętlą for na obiekty, ale to chyba byłby przerost formy nad treścią.

Twój kod $supportedAttribsIds = [CacheAtt::FEE['id']] można też ewentualnie ugryźć czymś w rodzaju gettera i odwoływać się w stylu: $attribId = CacheAttribute::getId(CacheAttribute::FEE) ale to chyba też przerost formy. Taka forma pseudogettera ma tą zaletę, że ukrywa logikę przed pozostałym kodem (hermetyzacja) i docelowo czymkolwiek będzie CacheAttribute:FEE - zawsze będzie działało poprawnie i ewentualny refactoring kodu będzie dotyczył tylko jednego miejsca.

Jako ciekawostkę dorzucę, że w PHP8 kod typu:

class Attrib {

public int $id;

public string $trKey;

__construct Attrib($id, $trKey)

{

    $this->id = $id;

    $this->trKey = $trKey;

}

}

można skrócić do:

class Attrib {

__construct Attrib(public int $id, public string $trKey)

{}

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/opencaching/opencaching-pl/pull/2304#discussion_r731658861, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWTAKB4QFSIFGPSJ7EN7G3UHUZJNANCNFSM5GFCBDWA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

andrixnet commented 2 years ago

There is no Aircraft nor AskOwner, 2 caches with attrib. OTHER. just remove the association between that attrib and the respective caches. Attribute is nonesense. (was some legacy from OCDE inspiration)