Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Allow to display custom Attributes in the Properties description #152

Merged
merged 15 commits into from
Aug 26, 2019

Conversation

Claymore1337
Copy link
Contributor

issue #151

In my project i have some attributes in the properties section which are not part of the JSONSchema. My requirment is to display also this properties.

So i would suggest to extend the the tool with an addtional parameter which except a list of
comma-separated values. This values will be used to read custom attributes of that propeties.

Roman Kunz added 4 commits July 24, 2019 17:33
A new console arg was added. This arg except a comma separated list. This list will be used to read
the custom attributes of an property

re adobe#151
@ghost
Copy link

ghost commented Jul 26, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ 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!

Copy link
Collaborator

@trieloff trieloff left a comment

Choose a reason for hiding this comment

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

Looks good, but the whitespace settings in your git client seem off.

Copy link
Collaborator

@trieloff trieloff left a comment

Choose a reason for hiding this comment

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

Could you also include an example in README.md and an integration test?

@Claymore1337
Copy link
Contributor Author

I think you mean the line endigs? Normally I use the recomend windows setting "core.autocrlf true". But if i enable this option on this project the properties list did not build correctly

image

So I have disable the convert. But when I use npm run commit it seems that it still do the convert.

And yes I will add the option to the Readme.md file and I will write a short test for it

@Claymore1337
Copy link
Contributor Author

@trieloff
Should I include it to all schemas ?In this block
image
Or should it get there own run with a singel file?

@trieloff
Copy link
Collaborator

Add it to the block you've outlined. I think it's good to have one example with minimum options and one with all options and hope that we never get options with cross-dependencies.

Add description into README.md file and add the new property into the tests

re adobe#151
@ghost
Copy link

ghost commented Jul 26, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ 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 Jul 26, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ 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!

@Claymore1337
Copy link
Contributor Author

@trieloff
I have added 2 fields to the schema example files and use this fields now in the test.

@ghost
Copy link

ghost commented Jul 29, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ 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!

Testfix after merging master

re adobe#151
@ghost
Copy link

ghost commented Jul 29, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ 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!

Copy link
Collaborator

@trieloff trieloff left a comment

Choose a reason for hiding this comment

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

We are getting there. I've found two typos and would suggest we declare p as an array property, which reduces the parsing burden.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cli.js Outdated
}
})
.alias('p', 'properties')
.describe('p', 'A comma separated list with custom properties which should be also in the description of an element.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.describe('p', 'A comma separated list with custom properties which should be also in the description of an element.')
.describe('p', 'A comma separated list with custom properties which should be also in the description of an element.')
.array('p')

This will use the built-in multi-value support in yargs, so that you can call jsonschema2md -p version -p test or jsonschema2md -p version test. I think that's more robust than trying to split the array yourself. (You'd have to adjust the rest)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trieloff
As far as I see the parser which is used did not have the function array.
image

So your suggestion will result in an error
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad, I forgot we are using optimist, not yargs.

Co-Authored-By: Lars Trieloff <lars@trieloff.net>
@ghost
Copy link

ghost commented Jul 30, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 95b2c97
    • ✖ 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!

Co-Authored-By: Lars Trieloff <lars@trieloff.net>
@ghost
Copy link

ghost commented Jul 30, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 95b2c97
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 61abcf1
    • ✖ 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!

1 similar comment
@ghost
Copy link

ghost commented Jul 30, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 95b2c97
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 61abcf1
    • ✖ 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!

@trieloff
Copy link
Collaborator

I've merged your other PR, now there's some cleanup work to do, but then we should be ready. Again, sorry for the confusion regarding the command line arguments.

Claymore1337 pushed a commit to Claymore1337/jsonschema2md that referenced this pull request Aug 15, 2019
Allow the use to disable the header over console args

re adobe#152
Merge branch 'parent-master' into costumProperties

re adobe#151
@ghost
Copy link

ghost commented Aug 15, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 95b2c97
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 61abcf1
    • ✖ 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!

@Claymore1337
Copy link
Contributor Author

@trieloff
I merged the master into the branch. It should be now ready to be merged.

@ghost
Copy link

ghost commented Aug 20, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 95b2c97
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 61abcf1
    • ✖ 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 Aug 20, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 95b2c97
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 61abcf1
    • ✖ 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!

@Claymore1337
Copy link
Contributor Author

@trieloff
Hi I wanted to update the Branch with the lastest changes. There I have seen that the dependency ajv was removed. But it is still used in the cli.js. This result in the following lintig error
image

How should I proceed? Should I add ajv again?

@trieloff
Copy link
Collaborator

Yes, please re-add it.

@ghost
Copy link

ghost commented Aug 26, 2019

There were the following issues with this Pull Request

  • Commit: 1a6beb1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 95b2c97
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 61abcf1
    • ✖ 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!

@trieloff trieloff merged commit 0634309 into adobe:master Aug 26, 2019
@trieloff
Copy link
Collaborator

Thank you for your contribution and your perseverance in the process, @Claymore1337

@trieloff
Copy link
Collaborator

🎉 This PR is included in version 3.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants