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

Make it easier to add issues for nodes with name location preference #422

Merged
merged 4 commits into from
Nov 13, 2023

Conversation

Sija
Copy link
Member

@Sija Sija commented Nov 12, 2023

No description provided.

@Sija Sija added the refactor label Nov 12, 2023
@Sija Sija added this to the 1.6.0 milestone Nov 12, 2023
@Sija Sija requested a review from veelenga November 12, 2023 00:56
@Sija Sija self-assigned this Nov 12, 2023
@Sija Sija enabled auto-merge November 12, 2023 00:59
@Sija Sija force-pushed the refactor-adding-issues-with-name-location branch from cdafec3 to e103282 Compare November 12, 2023 01:51
@Sija Sija force-pushed the refactor-adding-issues-with-name-location branch from e103282 to f984d83 Compare November 12, 2023 09:24
@Sija
Copy link
Member Author

Sija commented Nov 12, 2023

@veelenga This PR fixes CI, btw

@straight-shoota
Copy link
Contributor

Have you considered a different method name (such as add_name_issue)? I would argue that's a more explicit separation of behaviour and better than a parameter.

@Sija
Copy link
Member Author

Sija commented Nov 12, 2023

@straight-shoota I did considered some, yet it was the best I could come up with atm. I prefer named parameter, since it's a preference only, I see no point of having a dedicated method for it.

@straight-shoota
Copy link
Contributor

I would say the callsite is more succinct and easier to understand what it means. prefer_name_location is named after an internal detail, but naming_issue_for describes the purpose.

naming_issue_for node, MSG
#vs
issue_for node, MSG, prefer_name_location: true

@veelenga
Copy link
Member

I agree with @straight-shoota that naming_issue_for looks more understandable. Also, that amount of named params in the issue_for method makes it much more complicated.

@Sija
Copy link
Member Author

Sija commented Nov 12, 2023

Yeah, I agree, although in this case the parameter indicates a preference, not an unconditional behavior, like I've understood you've suggested. Also, naming is a pretty generic term, and doesn't reference name_location, which this feature is all about.

@Sija
Copy link
Member Author

Sija commented Nov 12, 2023

In this particular context all proposed names: i.e. add_name_issue and naming_issue_for are IMO too vague and unclear. name_location is a term already used, therefore familiar. Also, as I mentioned before, this is a setting which modifies the original behaviour, and so in my opinion is not suited for extraction to a different method.

@Sija Sija requested a review from veelenga November 13, 2023 17:09
Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

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

LGTM

@Sija Sija merged commit 0b225da into master Nov 13, 2023
4 checks passed
@Sija Sija deleted the refactor-adding-issues-with-name-location branch November 13, 2023 18:16
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.

None yet

3 participants