Skip to content

Conversation

@BigOHenry
Copy link

Added option for generating random name of uploaded file.

@Olicek
Copy link
Owner

Olicek commented Nov 20, 2015

Díky za PR. Měl bych k tomu pár poznámek.

  1. oprav prosím odsazení. Možná to je IDEčkem (používám PHPStorm), ale někde máš odszeno 2 taby a má být jen jeden atp.

  2. Nelíbí se mě, že můžu vygenerovat buď náhodný jméno, nebo přepsat existující soubor. Nevím jestli by to teď k něčemu bylo, ale líbilo by se mě, kdyby to bylo na sobě nezávislý. Tzn. Generování jména asi hodit někam bokem do private metody. Ta metoda se zaolá buď jednou (rewriteExistingFiles == TRUE) nebo v cyklu.

  3. v configu a setrmetoda by se mě víc líbila, kdyby se jmenovala setRandomFileName respektive randomFileName pro neon.

  4. v configu a setrmetoda by se mě víc líbila, kdyby se jmenovala setRandomFileName respektive randomFileName pro neon.

Myslíš, že bys to mohl zapracovat?

@BigOHenry
Copy link
Author

  1. Používám taky PHPStorm, ale tohle jsem dělal přes webUI gitu. Ale máš pravdu, měl jsem tam někde špatně odsazení. Mělo by to být OK.

  2. Úplně tomu nerozumím, protože podle mě se přepsání existujících souborů vzájemně vylučuje s náhodným jménem. Pokud budu chtít generovat náhodná jména, tak přece nikdy nebudu chtít, aby se přepisal již existující soubor pokud se zrovna trefí stejný název.

  3. Upraveno.

Jinak díky za trpělivost, je to můj první PR :-)

@Olicek
Copy link
Owner

Olicek commented Nov 21, 2015

1, 3 díky :-)

2 - asi máš pravdu, že se to nikdy nevyužije. Na druhou stranu mě nepřijde v pořádku, že nastavením jednoho parametru ovlivníš jinej parametr (rewriteExistingFiles vs randomFile). Kdyby to náhodou někdo někdy chtěl, tak to bude dost wtf moment si myslím.

Další věc je, že tam je teď 2x skoro stejnej kod pro nastavení toho jména souboru. Jednou se generuje náhodnej a jednou se rozloží a poskládá znova. To by se právě mohlo dát bokem a volat v cyklu podle toho jestli je rewriteExistingFiles TRUE nebo ne.

Koukal jsem, že to je tvůj první PR, seš dobrej, že ses odhodlal. Díky 👍

EDIT: Možná by dávalo smysl nedávat tam parametr randomFile, ale rewriteExistingFiles přidat další nastavitelnou hodnotu random?

@BigOHenry
Copy link
Author

Jo, to máš pravdu. Zkusím něco vymyslet a upravit to lépe.

@Olicek
Copy link
Owner

Olicek commented Jan 17, 2016

Budeš to dodělávat nebo to mám dotáhnout sám? Zatím to nepotřebuju, ale když už to máš skoro hotový, tak je škoda to nedotáhnout ;-)

@BigOHenry
Copy link
Author

Ahoj,
promiň, že jsem to zatím nedodělal, chvilku jsem na tom nedělal. Navíc
ani nevím jako to uchpit, obě části jsou sice podobné, ale nevím jak na
to... V projektu to teď mám tak jak jsem to posílal.

O.S.

Dne 17. 1. 2016 v 21:35 Petr Olišar napsal(a):

Budeš to dodělávat nebo to mám dotáhnout sám? Zatím to nepotřebuju,
ale když už to máš skoro hotový, tak je škoda to nedotáhnout ;-)


Reply to this email directly or view it on GitHub
#1 (comment).

@Olicek
Copy link
Owner

Olicek commented Jan 23, 2016

Dobře, díky za zprávu. Když budu mít čas, tak to zkusím nějak dotáhnout sám.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants