Add object enum #36

felipeplets · 2020-04-05T16:59:57Z

This PR depends on Adopt DRY principle and enhance performance #35
This implement an enum like object in order to suffice the discussion in Enable the type to be an enumerator instead of a type. #25

nivekcode · 2020-04-05T18:01:49Z

src/lib/generators/code-snippet-generators.ts

+    export type ${iconTypeName} = ${svgDefinitions
+    .map(({ typeName }, index) => `'${typeName}'${index === svgDefinitions.length - 1 ? '' : ' | '}`)
+    .join('')};`;
+  const typeObject = `


This means that currently, we would always generate both, an object for iteration and also a type and there's no option to disable it. I am just thinking of the best way to do this.

I personally would prefer to generate types by default. Then I would also offer the possibility to generate only the const or const and type. Some options like this

generateTypes: boolean (default: true); generateTypeObject:boolean (default: false);

@felipeplets what do you think?

I like the idea. Will push the change later today.

…t-enum

…bject

src/lib/options/config-collector.ts

felipeplets · 2020-04-06T04:43:21Z

@kreuzerk the options are created and work as we aligned.

The only point is that reading about commander I understood that to work as a boolean you can set --no-optionName and it will set the option as false, so as the default is true I've documented this way on the readme.

Also if --generateTypeObject is defined it will always generate the type as the object need it.

nivekcode · 2020-04-06T07:16:40Z

src/lib/options/args-collector.ts

@@ -10,6 +10,8 @@ export const setupCommander = () => {
  commander
    .version(packgeJSON.version)
    .option('-t --typeName <string>', 'name of the generated enumeration type', DEFAULT_OPTIONS.typeName)
+    .option('--no-generateType', 'prevent generating enumeration type', DEFAULT_OPTIONS.generateType)


--generateType instead of --no-generateType

nivekcode · 2020-04-06T07:17:27Z

README.md

@@ -201,6 +201,8 @@ Available configurations:
 | --version                 | type                    | default                                  | output the version number                                                                                                                                                       |
 | ------------------------- | ----------------------- | ---------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | typeName                  | string                  | myIcons                                  | name of the generated type                                                                                                                                                      |
+| no-generateType           | boolean                 | false                                    | prevent generating enumeration type                                                                                                                                             |


generateTypeinstead of no-generateTypeand it defaults to true

nivekcode · 2020-04-06T07:18:25Z

src/lib/generators/code-snippet-generators.ts

-  );
-  return typeHeader + types.join('');
+
+  if (convertionOptions.generateType || convertionOptions.generateTypeObject) {


I would remove the or check - I would say that the user should be able to generate either a type or an object or both.

nivekcode · 2020-04-06T07:24:10Z

Sorry, I just saw your comment now. All my comments in the PR correspond to your comment. 😅

I would still prefer to have the public flags like discussed. Users could still set the --generateType=false.

Why are the types needed if you generate the options?

felipeplets · 2020-04-06T12:34:28Z

Actually we can generate the object with the type, it's just less strict but can be done. I will change the code accordingly.

felipeplets · 2020-04-06T13:05:06Z

Regarding the config parameters, I've read the guide from commander before implementing and had a couple of findings that I would address in another PR, but I can list them bellow because I believe they can be beneficial to this topic.

The commander way of setting boolean parameters is:

syntax --paramName
if --paramName is not set the value will be the default value
if --paramName is set the value will be true
if --no-paramName is set the value will be false (the no- is automatically converted to false on the parameter paramName)

I understand this is different from Angular CLI syntax, which uses it's own parser: https://github.com/angular/angular-cli/blob/master/packages/angular/cli/models/parser.ts

If we want to implement following the same Angular syntax --paramName=true|false we can but we need to add a parser for the boolean values to make sure they are behaving as expected (I can address it in a next PR if you agree).

Also --paramName <paramValue> the paramValue is always a string if we don't parse it so --compileSources and --optimizeForLazyLoading only work now because the default is false but when we set them --optimizeForLazyLoading true they actually become 'true' which is truthy. So --compileSources false and --optimizeForLazyLoading false actually make them truthy which I consider to be a bug if they are meant to be boolean.

If you agree I can change this implementation to parse the same way Angular parses, have them both as --generateType <boolean> and --generateTypeObject <boolean> and in a later PR we can apply it to the existing boolean params.

nivekcode · 2020-04-06T14:40:17Z

@felipeplets Wow! Thanks a lot for that detailed explanation. 👍 Didn't know about this commander conventions.

I would prefer to parse them in the way Angular parses them. I think most developers are familiar with the Angular way and less with the commander way.

nivekcode · 2020-04-06T14:43:34Z

So, this means that once we change the parsing to the way Angular parses we have to do a breaking change? Are there still changes open for this branch or are we good to merge?

felipeplets · 2020-04-06T17:27:39Z

I don't see any breaking change.
I would say from the 3 implementations I did or am doing it all would result in a minor update.

Add --generateType and --generateTypeObject options to better control the auto-generated type
Enhance performance by adding parallel processing the icons
Fix --optimizeForLazyLoading and --compileSources to respect a false parameter

nivekcode · 2020-04-07T05:49:03Z

👍 So in order to merge this PR I would still adjust the following points:

Remove the or check for the type generation
Rename the no-generateType to generateType. I think it is cool if we keep all of our options sync - means all the argument configurations are the same as the configurations in package.json or .svg-to-tsrc

felipeplets · 2020-04-09T12:37:12Z

That is right @kreuzerk , this week is a bit busy for me so I will only be able to implement the changes over the weekend.

nivekcode · 2020-04-09T13:56:49Z

@felipeplets no stress - I can also help if you want - just let me know

…bject

felipeplets · 2020-04-11T19:49:50Z

@kreuzerk I've implemented the changes we agreed.

nivekcode · 2020-04-12T10:21:23Z

@felipeplets Awesome will go through them today and release the version today. Thanks a lot for the great work 👍

nivekcode · 2020-04-12T10:27:22Z

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Add object to simulate enum

cd18e59

felipeplets mentioned this pull request Apr 5, 2020

Enable the type to be an enumerator instead of a type. #25

Closed

nivekcode reviewed Apr 5, 2020

View reviewed changes

Felipe dos Santos added 2 commits April 5, 2020 22:04

Merge remote-tracking branch 'upstream/master' into feature/add-objec…

6bcce87

…t-enum

feat(config): add options to not generate type and to generate type o…

8bbec6f

…bject

felipeplets commented Apr 6, 2020

View reviewed changes

src/lib/options/config-collector.ts Outdated Show resolved Hide resolved

Update src/lib/options/config-collector.ts

12d8916

nivekcode requested changes Apr 6, 2020

View reviewed changes

feat(config): add options to not generate type and to generate type o…

9e120b3

…bject

nivekcode approved these changes Apr 12, 2020

View reviewed changes

nivekcode merged commit 7b86b3c into nivekcode:master Apr 12, 2020

nivekcode added the released label Apr 12, 2020

nivekcode linked an issue Apr 12, 2020 that may be closed by this pull request

Enable the type to be an enumerator instead of a type. #25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add object enum #36

Add object enum #36

felipeplets commented Apr 5, 2020

nivekcode Apr 5, 2020

nivekcode Apr 5, 2020

felipeplets Apr 5, 2020

felipeplets commented Apr 6, 2020

nivekcode Apr 6, 2020

nivekcode Apr 6, 2020

nivekcode Apr 6, 2020

nivekcode commented Apr 6, 2020 •

edited

Loading

felipeplets commented Apr 6, 2020

felipeplets commented Apr 6, 2020 •

edited

Loading

nivekcode commented Apr 6, 2020

nivekcode commented Apr 6, 2020 •

edited

Loading

felipeplets commented Apr 6, 2020

nivekcode commented Apr 7, 2020

felipeplets commented Apr 9, 2020

nivekcode commented Apr 9, 2020

felipeplets commented Apr 11, 2020

nivekcode commented Apr 12, 2020

nivekcode commented Apr 12, 2020

Add object enum #36

Add object enum #36

Conversation

felipeplets commented Apr 5, 2020

nivekcode Apr 5, 2020

Choose a reason for hiding this comment

nivekcode Apr 5, 2020

Choose a reason for hiding this comment

felipeplets Apr 5, 2020

Choose a reason for hiding this comment

felipeplets commented Apr 6, 2020

nivekcode Apr 6, 2020

Choose a reason for hiding this comment

nivekcode Apr 6, 2020

Choose a reason for hiding this comment

nivekcode Apr 6, 2020

Choose a reason for hiding this comment

nivekcode commented Apr 6, 2020 • edited Loading

felipeplets commented Apr 6, 2020

felipeplets commented Apr 6, 2020 • edited Loading

nivekcode commented Apr 6, 2020

nivekcode commented Apr 6, 2020 • edited Loading

felipeplets commented Apr 6, 2020

nivekcode commented Apr 7, 2020

felipeplets commented Apr 9, 2020

nivekcode commented Apr 9, 2020

felipeplets commented Apr 11, 2020

nivekcode commented Apr 12, 2020

nivekcode commented Apr 12, 2020

nivekcode commented Apr 6, 2020 •

edited

Loading

felipeplets commented Apr 6, 2020 •

edited

Loading

nivekcode commented Apr 6, 2020 •

edited

Loading