Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix array type #33

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix array type #33

wants to merge 3 commits into from

Conversation

Hhanen07
Copy link

@Hhanen07 Hhanen07 commented Oct 3, 2023

If we are having an attributes with an array type, it was shown as "Object[]" without specification of element's type of the array. I just modified the "if" condition to show the type of the elements in case of a Native type. As a result, it will be shown as String[] or Number[] etc.

I hope this modification are useful.
Screenshot 2023-10-03 at 08 09 38

@stefanosgk
Copy link

Hi! Thanks for the PR.
This would be a nice addition indeed.

A general objection I have with the PR is that you should try to follow the coding style of the repo and not enforce your own. For example you added semicolons, replaced single quotes with double quotes etc.

yarn.lock Outdated

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should undo these changes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still some changes in the yarn.lock file. Make sure you revert everything

}

interface SquareConfigsInterface {
squares: SquareConfig[]
squares: SquareConfig[];
logs?: string[];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding this you also have to update test/array-as-properties/fixture.json accordingly.
Did you try to run the tests?

Copy link

@stefanosgk stefanosgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you fix the issue mentioned here also #33 (comment)

Please remove any change that is not related to the feature.

@stefanosgk
Copy link

If you don't mind I could push a commit to update the tests. In that case make sure "Allow edits from maintainers" is checked

@Hhanen07
Copy link
Author

Hhanen07 commented Oct 5, 2023

Yes sure, I checked the option. You can apply the modifications required

@stefanosgk
Copy link

For some reason I cannot push to your branch due to permission issues.

In any case in order to address this issue we would have to revert most of your changes.

It would be easier and cleaner to do it in a new PR.
Do you mind if I open the PR myself?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants