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

Im Browser öffnen: intelligente Ersetzungslogik für Ticker, ISIN und WKN #740

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@sunnylars

sunnylars commented May 4, 2017

Im Browser öffnen: intelligente Ersetzungslogik für Ticker, ISIN und WKN

Die Reihenfolge der {} in dem Bookmark bestimmt die Priorität. Die erste Ersetzung, die in der Security einen Wert hat, wird genommen, alle weiteren verworfen.

Falls keine Ersetzung statt gefunden hat, wird der erste Wert in der Reihenfolge Ticker,Isin,wnk und Name, der in Security befüllt ist, hinten an die Url gehängt.

p.s.: ich bin 1. git und 2. github neuling. falls irgendwelche fehler hier sind, bitte einfach bescheid geben.

@sunnylars sunnylars changed the title from Issue 698 to Im Browser öffnen: intelligente Ersetzungslogik für Ticker, ISIN und WKN May 4, 2017

@buchen

This comment has been minimized.

Owner

buchen commented May 13, 2017

Hi @sunnylars,

Vielen Dank für den Change!

Ich bin allerdings nicht ganz glücklich, dass sich das Verhalten ändert. Bisher konnte man einfach eine URL konstruieren, in der sowohl der Name als auch die ISIN enthalten war. Ich weiß nicht wie oft / häufig das verwendet wird, aber ich sehe auch keine Grund warum man das jetzt ändern sollte.

Könnte man nicht sowas machen?
{isin} erst die ISIN, oder leer, falls es keine ISIN gibt
{isin,wkn} nimmt die ISIN, oder falls es keine ISIN gibt die WKN, oder eben leer

Mit dem "Komma-Syntax" ist dann auch die "eines dieser Werte" Semantik klar.

types.put("{tickerSymbol}", "getTickerSymbol");

This comment has been minimized.

@buchen

buchen May 13, 2017

Owner

Warum keine Lambda Function? Per Reflection geht auch, aber man bekommt keine Compilefehler mehr, kann nicht in der IDE umbenennen.

Sowas wie

        Map<String, Function<Security, String>> types = new HashMap<>();
        types.put("isin", Security::getIsin);

This comment has been minimized.

@sunnylars

sunnylars May 14, 2017

Ich muss gestehen, gehört habe ich schon von Lambda Functions, aber beruflich arbeite ich mit Java 7.
Aber ich lerne gerne, ich schaue mir das an und baue es um.

This comment has been minimized.

@buchen

buchen May 20, 2017

Owner

Vielleicht ist es auch einfacher die Werte direkt in die Map zu setzen - statt den Umweg über die Methode. Es ist ja nicht so als wären das teuere Methodenaufrufe die man nur machen möchte wenn tatsächlich eine Ersetzung stattfindet. Die Werte sind ja da. 😄

}
catch (NoSuchMethodException | SecurityException | IllegalAccessException | IllegalArgumentException | InvocationTargetException e)
{
e.printStackTrace();

This comment has been minimized.

@buchen

buchen May 13, 2017

Owner

Die Exceptions würde ich in RuntimeExcepition verpacke und weiterwerfen. Ich weiß in dem "name.abuchen.portfolio" Bundle kann man nicht einfach Fehler loggen. Aber die StackTrace mit printStackTrace landen quasi im Nirwana (der err output stream wird in eine Datei in den Configuration Folder umgeleitet). Dann lieber werfen und das UI Modul loggt einen Fehler.

Und: vermutlich fallen die alle weg wenn man die Reflection ausbaut.

}
if(!replacementDone){
url = pattern.replace("{tickerSymbol}", encode("")); //$NON-NLS-1$

This comment has been minimized.

@buchen

buchen May 13, 2017

Owner

Ist das nicht doppelt? Wenn es pattern gibt, dann dürfte das doch hier nicht mehr ersetzt werden können?

url = url.replace("{isin}", encode("")); //$NON-NLS-1$
url = url.replace("{wkn}", encode("")); //$NON-NLS-1$
url = url.replace("{name}", encode("")); //$NON-NLS-1$
if(security.getTickerSymbol() != null && security.getTickerSymbol().length()>0)

This comment has been minimized.

@buchen

buchen May 13, 2017

Owner

Ich würde die "magic" weglassen. Wenn man kein Pattern angibt, dann ist eben auch Wert in der URL. Einfach Werte hinten an die URL hängen finde ich zu "magic".

Lars Hercher added some commits May 20, 2017

buchen added a commit that referenced this pull request May 21, 2017

Added intelligent replacement logic for bookmarks
Issue: #740
Signed-off-by: Lars Hercher <it@larshercher.de>
[cherry-picked and squashed commits]
Signed-off-by: Andreas Buchen <andreas.buchen@gmail.com>

buchen added a commit that referenced this pull request May 21, 2017

@buchen

This comment has been minimized.

Owner

buchen commented May 21, 2017

Hi @sunnylars,

die Änderung ist drin - siehe e235583 👍

Ich habe noch einen kleinen Change hinterher geschoben:

  • ein Kontextmenü mit den Ersetzungen --> eine Art indirekte Dokumentation welche Ersetzungen möglich sind
  • Neue "tickerSymbolPrefix": Dazu habe ich schon mal eine Anfrage bekommen weil es eine Webseite gibt, die nicht mit ganzen symbol umgehen kann
  • Noch ein paar neue Tests --> darum musst ich die Logik noch mal leicht anpassen. Bei einer URL wie "isin={isin}" wurden beide Teile ersetzt weil zunächst die Klammern entfernt wurden. Also habe ich die Ersetzung direkt nach dem Matcher gesetzt.

@buchen buchen closed this May 21, 2017

buchen added a commit that referenced this pull request May 21, 2017

@buchen

This comment has been minimized.

Owner

buchen commented May 21, 2017

Und weil wir gerade dran waren: Ich habe noch die Möglichkeit eingebaut, dass man auch beliebige Attribute der Wertpapiere für die Ersetzung nutzen kann. Das ging relative einfach weil Du die generische Ersetzungslogik gebaut hast.

https://github.com/buchen/portfolio/blob/master/name.abuchen.portfolio/src/name/abuchen/portfolio/model/Bookmark.java#L58-L64

@sunnylars

This comment has been minimized.

sunnylars commented May 21, 2017

Ok, danke Dir.

Sieht gut aus. Besonders das mit dem Kontextmenu gefällt mir.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment