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

feat: consistent paths in messages #867

Merged
merged 16 commits into from
Dec 23, 2019
Merged

feat: consistent paths in messages #867

merged 16 commits into from
Dec 23, 2019

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Dec 19, 2019

Addresses #815

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

@P0lip P0lip added the enhancement New feature or request label Dec 19, 2019
@P0lip P0lip self-assigned this Dec 19, 2019
@P0lip P0lip force-pushed the fix/consistent-paths branch from 4679de6 to 7cf4062 Compare December 20, 2019 11:25
@P0lip P0lip changed the title feat: consistent paths feat: consistent paths in messages Dec 20, 2019
src/__tests__/linter.test.ts Outdated Show resolved Hide resolved
@P0lip
Copy link
Contributor Author

P0lip commented Dec 23, 2019

@marbemac @philsturgeon
the last remaining question is whether we are making paths configurable - right now we always try to display the property name, but some want to have the whole path printed.
What do you think?

@philsturgeon
Copy link
Contributor

philsturgeon commented Dec 23, 2019 via email

@marbemac
Copy link
Contributor

whether we are making paths configurable

Suggest we punt this to later - not convinced we need it so might as well save the time now :).

@P0lip
Copy link
Contributor Author

P0lip commented Dec 23, 2019

If I had to guess at the use case for people who want the whole path, they are probably using it programmatically do if we make it available in the object (and it can be inserted into the message string via a {{path}} variable) then both JS and CLI users should be happy. Then we can keep it property whenever possible by default.

We do expose {{path}} now, so your use case would be covered :)

@P0lip P0lip marked this pull request as ready for review December 23, 2019 18:26
Copy link
Contributor

@marbemac marbemac left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me 👍.

@P0lip P0lip merged commit 3824df4 into develop Dec 23, 2019
@P0lip P0lip deleted the fix/consistent-paths branch December 23, 2019 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants