-
Notifications
You must be signed in to change notification settings - Fork 41
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
[TASK] improve output (filtering, fix links type, new outputs nodelist for freifunkapi and meshviewer-ffrgb v10 NextGen) #63
Conversation
281d79b
to
7da7b07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warum akzeptieren die filter*
Funktionen einen nil-Parameter und warum geben sie etwas anderes als bool
zurück? Bitte überdenken!
output/all/filter.go
Outdated
xA float64 | ||
xB float64 | ||
yA float64 | ||
yB float64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wieso unterschiedliche Bezeichnungen? Hier x
und y
und dort latitude
und longitude
?
Vier Punkte in einem zweidimenionalen Raum können was ganz anderes sein....
output/all/filter.go
Outdated
return node | ||
} | ||
} else { | ||
if location := nodeinfo.Location; location == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitte entschachteln
output/all/filter.go
Outdated
return nil | ||
} | ||
|
||
func filterLocationInArea(node *runtime.Node, a area) *runtime.Node { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warum sollte nil
an diese Funktion übergeben werden?
output/all/internal.go
Outdated
for outputType, outputRegister := range output.Adapters { | ||
outputConfigs, ok := allOutputs[outputType].([]map[string]interface{}) | ||
if !ok { | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Einfach ignorieren, wenn nicht ok
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Du meinst man sollte erst aus der map lesen und dann den Objekttypen wandlen ( wobei dann ein panic geworfen wird?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Die Frage ist: Erwartet ein Anwender von Yanic, dass eine fehlerhafte Konfiguration stillschweigend akzeptiert wird? Was ist hilfreich?
output/all/internal.go
Outdated
filteredNodes := nodes | ||
if config, ok := o.filter[i]; ok { | ||
filteredNodes = config.filtering(nodes) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was wenn nicht ok
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPS, ist mir auch noch gestern eingefalleeingefallen, das dann der owner nicht standartdmäßig weggefilter wird.
output/internal_test.go
Outdated
|
||
Close() | ||
|
||
time.Sleep(time.Millisecond * 12) // to reach timer.Stop() line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hier brauchst du nicht schlafen, wenn du WaitGroups benutzt.
|
||
func Register(configuration map[string]interface{}) (output.Output, error) { | ||
var config Config | ||
config = configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weshalb struct kopieren?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damit ein typumwandlung stattfinden - meinst das sollte auch im funktionskopf möglich sein?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope :(
var nodeFormats = map[int64]nodeBuilder{ | ||
1: BuildNodesV1, | ||
2: BuildNodesV2, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wie wäre es mit zwei output-Plugins für die Versionen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dann wären zweimal der graph builder notwendig,
auch wenn der vom anderen Plugin importierbar wäre.
(Ist für mich nicht DRY)
Außerdem ist es für die History der Meshviewer interessant. (Die Version 1 und 2 stammen ja aus ffnord)
output/meshviewer-ffrgb/output.go
Outdated
return enable.(bool) | ||
} | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diese Funktion gibt es vier mal in zwei unterschiedlichen Schreibweisen. Geht das auch DRY?
output/all/filter.go
Outdated
if v, ok := f["no_owner"]; ok { | ||
return v.(bool) | ||
} | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
das geht kürzer
Danke für die Anregungen |
Codecov Report
@@ Coverage Diff @@
## master #63 +/- ##
===========================================
+ Coverage 59.43% 73.36% +13.93%
===========================================
Files 24 37 +13
Lines 843 1164 +321
===========================================
+ Hits 501 854 +353
+ Misses 323 299 -24
+ Partials 19 11 -8
|
|
output/internal_test.go
Outdated
Close() | ||
assert.Equal(1, conn.CountSave) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hab keine Erklärung, wie hier jemals eine 0
entstehen kann ->
https://travis-ci.org/FreifunkBremen/yanic/jobs/293724350#L529
output/meshviewer-ffrgb/struct.go
Outdated
Enabled bool `json:"enabled,omitempty"` | ||
Branch string `json:"branch,omitempty"` | ||
Enabled bool `json:"enabled"` | ||
Branch string `json:"branch"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think branch is fine omitempty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bitte wirf das template Output-Plugin raus. Der Nutzen erschließt sich mir nicht.
616596d
to
da6096e
Compare
0041925
to
0bed780
Compare
…t for freifunkapi and meshviewer-ffrgb v10 NextGen)
enable = true | ||
path = "/var/www/html/meshviewer/data/meshviewer.json" | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Du verwendest mal doppelte und mal einfache Klammern für die Sections. Hat das einen Grund?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nö xD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doch, das sind Array-Einträge mit "[[...]]" dadurch können mehrere datenbanken oder outputs configuriert werden.
z.B. output ohne Filter und eine mit Filter, das owner angezeigt wird (für eine hidden Meshviewer)
oder ein Meshviewer nur für Walle, um die auf deren Seite einzubinden.
... es gibt viele Möglichkeiten
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dann erkläre das bitte in der Config-Datei.
…t for freifunkapi and meshviewer-ffrgb v10 NextGen) - #63
should fix also
nodelist
(the minimal version for the freifunkapimeshviewer-ffrgb
(Support new meshviewer(-ffrgb) file-format #74)