Skip to content

Conversation

@SachsKaylee
Copy link
Collaborator

@SachsKaylee SachsKaylee commented Sep 5, 2018

Currently the functionValue option is ignored for functions nested in complex data structures, leading to outputs potentially looking like this:

<Form
  data={{}}
  elements={[
    {
      allowCustomTags: false,
      key: 'tagList',
      name: 'TagList',
      tags: [
        'tag-1',
        'tag-2',
        'tag-3'
      ],
      type: function noRefCheck() {} // <<--
    }
  ]}
  onSubmit={() => { ... }} // <<--
/>

Options:

{ showFunctions: false, functionValue: () => "() => { ... }" }

This PR changes this behavior:

<Form
  data={{}}
  elements={[
    {
      allowCustomTags: false,
      key: 'tagList',
      name: 'TagList',
      tags: [
        'tag-1',
        'tag-2',
        'tag-3'
      ],
      type: () => { ... } // <<--
    }
  ]}
  onSubmit={() => { ... }} // <<--
/>

To achieve this without creating duplicate code I added a new formatter called formatFunction and added the respective unit tests for it. The formatPropValue formatter has been updated to use this new formatter.

Edit, some feedback on my developer experience, feel free to ignore:
You probably want to add a hint about enforcing a certain commit style in your Readme or a special contribution doc and mention what kind of messages are valid. Tracking down a guide for angular style commits when committing to React project was ... unexpected :)
Also the precommit formatter script kept breaking my unit tests. I'm aware that this is an edge case since the code layout typically doesn't affect logic, but especially in libraries like this, it does. I didn't understand why my tests suddenly failed after committing since I did not expect git commit/push to reformat my code.

@ghost
Copy link

ghost commented Sep 5, 2018

There were the following issues with this Pull Request

  • Commit: 4e5b674
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 93b442e
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented Sep 5, 2018

There were the following issues with this Pull Request

  • Commit: 0c421f6
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@SachsKaylee
Copy link
Collaborator Author

What's the status on this?

Feel free to reject the PR if its not something you want/need, but I'd really appreciate some kind of feedback.

@vvo
Copy link
Contributor

vvo commented Sep 25, 2018

@PatrickSachs Sorry I have not been maintaining this, would you like to be the new maintainer?

@SachsKaylee
Copy link
Collaborator Author

@vvo Certainly.

I'd need some additional info on how this repository is managed, what my permissions are, etc, but since I expect to be using a library that directly depends on this quite a bit in the future this would simplify a lot of things.

@vvo
Copy link
Contributor

vvo commented Sep 27, 2018

An invite link was sent to you, what's your npm username too? So you can publish.

About how the repository is managed, everything is documented in the README (about publishing). Let me know if you have other questions.

@SachsKaylee
Copy link
Collaborator Author

Thank you very much. My npm name is patrick.sachs. I'll ask if any questions arise.

@vvo
Copy link
Contributor

vvo commented Sep 28, 2018

Added you.

I would be fine removing the precommit hook, adding it to the readme, and using https://github.com/apps/semantic-pull-requests maybe

@vvo vvo merged commit 94d1aeb into algolia:master Sep 28, 2018
@SachsKaylee SachsKaylee deleted the functionValue branch September 28, 2018 18:27
@SachsKaylee
Copy link
Collaborator Author

This sounds like a good idea. I've requested the installation of the app into this repository.

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