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.

Sign up for GitHub

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

Jump to bottom

cut as default #399

Closed
wants to merge 14 commits into from
Closed

cut as default #399

wants to merge 14 commits into from

Conversation

lucasborin
Copy link
Member

No description provided.

@lucasborin lucasborin linked an issue May 14, 2021 that may be closed by this pull request
Consider cut as default name #369
Open
@lucasborin lucasborin marked this pull request as ready for review May 14, 2021 13:43
@lucasborin lucasborin requested a review from a team as a code owner May 14, 2021 13:43
@lucasborin
Copy link
Member Author

@estevao-schultz-neto-SAP: Would you mind reviewing the new documentation?

@lucasborin
Copy link
Member Author

image

cut in the method name should be ignored.

@lucasborin lucasborin marked this pull request as draft May 14, 2021 17:20
estevao-schultz-neto-SAP
estevao-schultz-neto-SAP reviewed May 17, 2021
View reviewed changes
Copy link
Contributor

@estevao-schultz-neto-SAP estevao-schultz-neto-SAP left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello Buddy, I don't think this check should be "enforced" in the sense that someone will need to use the pseudo-comment in case he/she has used a MEANINGFUL name. I think we have a mismatch here. From the text: "...In a test class, the code under the test can be represented using a meaningful name, or cut as a default..." either CUT or a MEANINGFUL name should be accepted. But what is a meaningful name then? Subjective...

Long story short: IMHO, this CHECK will mostly be used as a NOTIFICATION in case someone is not using "cut" (and in case the team agreed so -- team decision).

Moreover, I would change the description so that: _"...In a test class, the code under the test can be represented using a meaningful name or cut as a default. If your team decided to use 'cut', this check can be helpful on identifying deviations.". _

We talk about it further.

ghost
ghost previously approved these changes Jun 2, 2021
View reviewed changes
Copy link

@ghost ghost 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 to me

@lucasborin
Copy link
Member Author

Hello Buddy, I don't think this check should be "enforced" in the sense that someone will need to use the pseudo-comment in case he/she has used a MEANINGFUL name. I think we have a mismatch here. From the text: "...In a test class, the code under the test can be represented using a meaningful name, or cut as a default..." either CUT or a MEANINGFUL name should be accepted. But what is a meaningful name then? Subjective...

Long story short: IMHO, this CHECK will mostly be used as a NOTIFICATION in case someone is not using "cut" (and in case the team agreed so -- team decision).

Moreover, I would change the description so that: _"...In a test class, the code under the test can be represented using a meaningful name or cut as a default. If your team decided to use 'cut', this check can be helpful on identifying deviations.". _

We talk about it further.

All the checks are NOTIFICATION by default so, as you mentioned, it is up to the team to decide if they want to follow cut as the main default or not. The check will just support these ones. Besides, I will enhance the documentation as you mentioned.

Base automatically changed from 1.15.0 to master August 12, 2021 14:05
@lucasborin lucasborin changed the base branch from master to 1.17.0 November 1, 2021 13:25
@lucasborin lucasborin dismissed ghost ’s stale review November 1, 2021 13:25

The base branch was changed.

Base automatically changed from 1.17.0 to master February 8, 2022 12:37
@bjoern-jueliger-sap
Copy link
Member

Closing this as no longer relevant

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

@estevao-schultz-neto-SAP estevao-schultz-neto-SAP estevao-schultz-neto-SAP left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider cut as default name
3 participants
@lucasborin @bjoern-jueliger-sap @estevao-schultz-neto-SAP