-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
…el objects from json
This reverts commit 6704dd8.
lib/gyro/utils/file_utils.rb
Outdated
Gyro::Error::exit_with_error('You need to specify right template directory using --template option (see --help for more info)') | ||
end | ||
else | ||
template_dir_to_test = Pathname.new("data/templates/") + template_dir_to_test |
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 coup ce qu'on avait vite-fait vu avec DataDir
ça marche pas ?
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.
Alors pour ce point j'ai creusé un peu sur DataDir
et je n'arrive pas à l'utiliser.
Cela s'utilise comme ça Gem.datadir('gem_name')
. L'utilité de ce répertoire est de fournir de la data en public. Pour que par exemple d'autres scripts ruby puisse piocher dans le datadir
. Parfait sauf que je n'ai pas réussi à l'utiliser en interne dans gyro.
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.
Ah mince. Que retourne Gem.datadir('gyro')
, ça retourne Nil? Peut être que c'est parce qu'il va chercher dans le dossier système où la gem est installée (plutôt que le répertoire local où tu es en train de bosser sur 'gyro') et que c'est pour ça qu'il ne le trouve pas ? Ou bien peut être faut il déclarer son chemin relatif quelque-part genre dans la gemspec pour qu'il sache où le chercher (et que le nom "./data" pour ce répertoire n'est alors pas automatiquement inféré)?
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.
Je vais encore investiguer. J'avais pas pensé au gemspec
. Merci du conseil.
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.
Bon de ce que je lis de l'implémentation (https://apidock.com/ruby/Gem/datadir/class), la méthode Gem.datadir(gem_name)
fait ceci:
def self.datadir(gem_name)
spec = @loaded_specs[gem_name]
return nil if spec.nil?
File.join spec.full_gem_path, "data", gem_name
end
Autrement dit ça prend le chemin où est installé la gem (du coup quand tu testes en local je sais pas ce que ça donne, mais je pense que ça marche quand même puisque tu as mis "./lib" dans le LOAD_PATH et que tu as "require 'gyro'" ainsi…? Mais bon j'en suis pas complètement sûr), et ça concatène ça avec "data/#{gem_name}". Donc on serait sensé faire un dossier "gyro/data/gyro/template" pour le faire fonctionner? Bon en plus j'ai un peu l'impression que c'est déprécié, du coup pas sûr que ça vaille le coup de se prendre la tête là dessus.
Le principal c'est de vérifier que ça continuera de marcher une fois installé sous forme de gem, en particulier si on fait gem install gyro
est-ce que ça installe aussi le dossier data/templates et est-ce que le code pour trouver son chemin marche toujours comme il marchait lors des tests en local.
@@ -41,6 +41,14 @@ def initialize(relationship_xml, entity_name) | |||
search_for_error | |||
end | |||
|
|||
def to_h | |||
return { 'entity_name' => entity_name, 'name' => name, 'type' => "#{type}", |
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.
Pourquoi faire une interpolation avec "#{type}"
plutôt que d'utiliser type
directement comme pour les autres (ou type.to_s
au pire si ce n'est pas une string mais que tu veux en faire une string)?
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.
My bad. J'ai dû oublié de virer ça. Je me rappelle plus pourquoi mais je pense pas que ça soit utile. Erreur de débutant. :)
case TypeOne = "type_one" | ||
case TypeTwo = "type_two" | ||
case TypeThree = "type_three" | ||
case typeOne = "TypeOne" |
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.
C'est normal que maintenant ça soit bindé à la rawValue "TypeOne"
(pour quand tu parseras en JSON), plutôt que "type_one"
comme c'était le cas pour la fixture avant? C'est le comportement du template qui a changé?
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.
Dans mon template la génération de la rawValue des enums
est faite avec la JSONKey
si il y en a une sinon on prend la nameValue. Dans le enum.xcdatamodel
il n'y a pas de JSONKey
associée pour l'enum type
, donc mon template n'a pas pu en générer. Je me rappelle d'une chose un peu bizarre dans l'ancien code qui construisant quand même une rawValue
en snake_to_camel_case
. Est ce utile ? Je peux le faire si besoin.
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.
Tu veux dire que le type_one
qu'il y avait avant était totalement calculé par code via un snake_to_camel_case
?
Bizarre qu'on ait fait ça je vois pas trop la raison… (à part si c'est parce que c'était la convention utilisée sur le projet monoprix pour lequel dbgen a été créé et que du coup on a hardcodé ça à l'époque…?)
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.
Faudrait que je recherche mais en tout cas ça ne vient pas du .xcdatamodel
car il ne contient pas la clé type_one
mais une nameValue TypeOne
.
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.
Normalement ça ça ne viendrait pas tant de la clé JSONKeyPath (qui donne le nom de la clé JSON pour l'attribut que tu cherches à décider, pour la clé dans le JSON quoi) mais plutôt de la clé JSONValues
(qu'il faut renseigner quand tu mets aussi la clé enumValues
), qui est sensé lister toutes les valeurs possibles qu'on peut rencontrer pour cet attribut et les faire correspondre avec la liste des enumValues
. Cf README
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.
https://github.com/NijiDigital/gyro/blob/master/spec/fixtures/xcdatamodel/enum.xcdatamodel/contents
Dans le contents on voit bien qu'il n'y a même pas de JSONKeyPath
. Il y a juste enumType
et enumValues
. Moi en tout cas j'ai fait les templates avec un fallback sur les enumValues
au cas où il n'y aurait pas de JSONValues
.
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.
Et donc on voit que la transformation a pu se faire que part code. Faudrait que je cherche la fonction. Mais je ne sais pas pourquoi cette transformation.
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.
Ça vient apparemment de ce code: https://github.com/NijiDigital/gyro/blob/master/lib/gyro/realm/swift/enum_generator.rb#L58-L62
Si on n'a pas en JSONValues on les construit à partir des enumValues qu'on underscore.
Je ne sais pas d'où ça vient et pourquoi on a fait ça (surtout, pourquoi on les underscores, c'est un peu un choix arbitraire…). A la limite si on a pas de JSONValues on pourrait alors prendre les enumValues directement, ça aurait du sens, mais pourquoi prendre les enumValues et les passer en underscore, ça… je pense que c'est un reliquat des conventions Monoprix et qu'on peut s'en débarrasser, pour utiliser les enumValues telles quelles si pas de JSONValues effectivement du coup, un peu comme tu as l'air d'avoir fait vu le nouveau résultat dans la fixture donc.
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.
Ok moi ça me va aussi. Je pense que ça sera plus simple de compréhension pour l'utilisateur. S'il veut faire un traitement spécifique il pourra le faire dans un template custom.
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.
Tout à fait. Ou bien lister explicitement tous les cas dans son xcdatamodel en précisant explicitement le JSONValues si elles sont différentes des enumValues.
data/templates/decodable/root.liquid
Outdated
{% assign attributeKey = attribute.name -%} | ||
{%- if attribute.transformer.empty == false -%} | ||
{%- assign attributeKey = attribute.json_key_path -%} | ||
{%- endif -%} |
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.
Je ne suis pas sûr de bien comprendre la logique ici. La variable attributeKey
est affectée à json_key_path
… seulement s'il y a un transformer? Quel rapport ? Pourquoi s'il n'y a pas de transformer on ne prend pas la valeur de l'attribut JSONKeyPath mais le nom de l'entité? C'est nouveau comme comportement…
Je ne sais plus ce qu'on avait décidé comme comportement pour dbgenerator, s'il n'y a pas la clé JSONKeyPath est-ce qu'on décide alors d'ignorer la propriété lors du parsing JSON, et on ne va même pas essayer de la lire du JSON… ou est-ce qu'on disait qu'on supposait alors que la clé du JSON état dans ces cas là sensée avoir le même nom que l'attribut ? (mais dans ce cas pouvait-on indiquer pour un attribut donné qu'on ne souhaitait pas du tout l'include dans le décodage JSON, au cas où ce genre de cas se présentait sur un projet?)
En tout cas il faut garder le même comportement, et utiliser la clé JSONKeyPath si elle est présente… qu'il y ait un transformer custom ou non
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.
C'est un oublie de correction. Normalement c'est if attribute.json_key_path.size > 0
. Je vais corriger ce souci.
Il y a un problème. |
Je reviens sur une remarque que tu as fait au sujet du |
lib/gyro/utils/file_utils.rb
Outdated
@@ -26,4 +28,44 @@ def self.write_file_with_name(dir, name_file, content) | |||
file_path = File.expand_path(name_file, dir) | |||
File.write(file_path, content) | |||
end | |||
|
|||
def self.templates_dir | |||
Pathname.new(File.dirname(__FILE__)) + '../../' |
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.
Either:
- include the
/templates
part in that path if you name that methodtemplates_dir
, so that it actually indeed ponts to the templates directory - or name that method
self.lib_dir
if you're not pointing to the actual template dir but the lib dir
(I personally prefer the first solution, keeping that name and pointing to lib/templates
)
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.
Renaming suggestions
lib/gyro/liquidgen/generator.rb
Outdated
|
||
def generate_root_template(xcdatamodel) |
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'd rather rename that generate_entities_files
or generate_entities_from_root_template
, as it generates files from templates, it doesn't generate the root template itself.
lib/gyro/liquidgen/generator.rb
Outdated
Gyro.write_file_with_name(@output_dir, enum_filename, output) | ||
end | ||
|
||
def root_template_rendering(context) |
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.
Rename that render_root_template(context)
?
lib/gyro/liquidgen/generator.rb
Outdated
.gsub(/^ +$/, '') | ||
end | ||
|
||
def filename_rendering(context) |
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.
rename that render_filename(context)
?
lib/gyro/liquidgen/generator.rb
Outdated
filename_template.render(context).chomp | ||
end | ||
|
||
def enum_template_rendering(context) |
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.
Rename that render_enum(context)
?
lib/gyro/liquidgen/generator.rb
Outdated
.gsub(/^ +$/, '') | ||
end | ||
|
||
def enum_filename_template_rendering(context) |
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.
Rename that render_enum_filename
?
bin/gyro
Outdated
end | ||
opts.on('-x', '--annotations', 'To tag code with Android support annotations according to optional/non optional fields') do | ||
options[:annotations] = true | ||
end |
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.
We need to document somewhere (in some Markdown file documenting each template?) how to pass those params for Android now, i.e. what are the --param
names to use (and that are used by the Android templates provided with gyro) to achieve the similar features now
With all those new commits, where are we on the TODO list I've written in #16 (comment) ? Which items are now checked? |
@StevenWatremez could you please copy/paste the remaining tasks from the TODO list from #16 (comment) and update it accordingly regularly (by changing Note that once you've done that comnent-with checkboxes, you'll be able to simply click inside a checkbox to check it (you might not be able to do that on my comment because you can only edit yours, hence the benefit of duplicating that list in your own comment to be able to click the checkboxes! (For more info, see https://help.github.com/articles/about-task-lists/) |
@StevenWatremez I've created a GitHub Project to follow this big refactoring in a more fine-grained manner: https://github.com/NijiDigital/gyro/projects/1 Don't hesitate to add any missing card I might have forgotten, and don't forget to drag & drop the cards to move them in the right column when you find time to work on each task. The only card remaining before we can consider merging that PR into a dedicated |
Thanks, Projects seems to be an excellent feature on github to manage TODO's. Better than issues. |
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.
2 small changes for clarity, after that I'm good for merging!
{%- capture annotation -%}{%- if params.support_annotations == true -%} | ||
{%- if params.use_wrappers == true or attribute.enum_type.size > 0 or primitives == "false" -%} | ||
@android.support.annotation.{% if attribute.optional == true %}Nullable{% else %}NonNull{% endif -%} | ||
{%- if attribute.support_annotation.size > 0 %}-{% endif -%} |
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.
Nitpicking: I'd rather use a comma (,) than a - for this hack, feels less hacky at least, as , is a common separator for list items
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.
Change the separator char to "|" as comma are used in Android annotations (ex : @android.support.annotation.IntRange(from=0,to=255)). See commit aff6099
{%- endif -%} | ||
{%- endif -%} | ||
{%- endcapture -%} | ||
{%- capture primitives -%} |
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.
For clarity, please rename that variable isPrimitive
rather than primitives
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.
Done see commit 9b01178
Decodable template is under tests and discussions.
Java template was not implemented. It will be for a next PR. So you can still use Java generation like before.
This will fix both #3 and #8