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

cEP-0005.md: Defines way to name aspects and settings #67

Closed
wants to merge 1 commit into from
Closed

cEP-0005.md: Defines way to name aspects and settings #67

wants to merge 1 commit into from

Conversation

EverWinter23
Copy link
Contributor

@EverWinter23 EverWinter23 commented Mar 19, 2017

An aspect should just talk about a property of something, not qualifying it. Don't name it TooLong but Length.
Then give it settings, e.g. Metadata.CommitMessage.Shortlog.Length can have a setting max_shortlog_length.

## Settings
Copy link
Member

Choose a reason for hiding this comment

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

I think its better we have definition of tastes instead of settings. Moreover which issue is this related to? Have a proper commit message.

Copy link
Member

Choose a reason for hiding this comment

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

@EverWinter23 As @Techievena mentioned it's really important to rename settings to tastes, since the latter is the new official term.

@meetmangukiya
Copy link
Member

Squash the commits

@EverWinter23
Copy link
Contributor Author

Done

Copy link

@jack17529 jack17529 left a comment

Choose a reason for hiding this comment

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

No commit message body , please read how to wright good commit messages.

Copy link

@yashtrivedi96 yashtrivedi96 left a comment

Choose a reason for hiding this comment

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

Using bulleted list will make it better. Apply bulleted list for Aspects and Settings.
eg This "Then give it settings, e.g. Metadata.CommitMessage.Shortlog.Length can have a setting max_shortlog_length." is not reflected on a new line.

-item1
-item2
will be reflected as

  • item1
  • item2

@jack17529
Copy link

jack17529 commented Mar 23, 2017

@yashtrivedi96 is there any place where we can check these cool things about bulleted list showing and etc as my 1 doc issue is still floating.It would be very helpful.

@yashtrivedi96
Copy link

@yashtrivedi96
Copy link

That is for rst

@yashtrivedi96
Copy link

Copy link
Member

@userzimmermann userzimmermann left a comment

Choose a reason for hiding this comment

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

settings --> tastes

An aspect should just talk about a property of something, not qualifying it. Don't name it TooLong but Length.
Then give it settings, e.g. Metadata.CommitMessage.Shortlog.Length can have a setting max_shortlog_length.

## Settings
Copy link
Member

Choose a reason for hiding this comment

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

@EverWinter23 As @Techievena mentioned it's really important to rename settings to tastes, since the latter is the new official term.

@EverWinter23
Copy link
Contributor Author

will make the changes

Copy link
Member

@userzimmermann userzimmermann left a comment

Choose a reason for hiding this comment

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

#69 :)

@@ -124,6 +124,15 @@ class RedundancyBear(LocalBear):
yield Result.from_values(aspect=aspect, ...)
```

## Aspects
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the terms aspect and aspects lower case according to #69

@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@userzimmermann
Copy link
Member

@EverWinter23 Still working on this or want someone else to take over? :)

@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

2 similar comments
@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

5 similar comments
@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@userzimmermann
Copy link
Member

@EverWinter23 Closing this in favor of #72 , which is based on your PR! Thanks for your work :)

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

Successfully merging this pull request may close these issues.

8 participants