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

Remove Scala procedure syntax #4964

Closed
wants to merge 1 commit into from
Closed

Remove Scala procedure syntax #4964

wants to merge 1 commit into from

Conversation

joan38
Copy link
Contributor

@joan38 joan38 commented May 3, 2018

This removes the Scala procedure syntax from the codebase as this is deprecated since Scala 2.11:
scala/scala#3076

Let me know your thoughts.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

NoValInForComprehension
RemoveXmlLiterals
ProcedureSyntax
ExplicitUnit
@joan38 joan38 changed the title Applying some refactoring rules to Scala files Applying some refactoring rules to the Scala files May 4, 2018
@joan38 joan38 changed the title Applying some refactoring rules to the Scala files Apply some refactoring rules to the Scala files May 4, 2018
@joan38 joan38 changed the title Apply some refactoring rules to the Scala files Remove Scala prodecure syntax May 4, 2018
@joan38
Copy link
Contributor Author

joan38 commented May 4, 2018

This is ready to go. I will rebase once I have a first feedback.

@joan38
Copy link
Contributor Author

joan38 commented May 8, 2018

@guozhangwang @debasishg Any thoughts on this?

@guozhangwang
Copy link
Contributor

cc @ijuma for reviews.

@ijuma
Copy link
Contributor

ijuma commented May 9, 2018

Thanks for the PR. I think we eventually want to do something like this, but it's a bit invasive as it makes it harder to backport changes to previous branches and it introduces conflicts to in-progress PRs. It may make sense to discuss the usage of scalafix in the project and which rules we want to apply, etc. There is a JIRA for using scalastyle, but we never took that to completion.

@joan38
Copy link
Contributor Author

joan38 commented May 9, 2018

Note this is not introducing scalafix (yet). I'm waiting on 0.6 to be released to get some stuff sorted out.
See the similar PR about formatting that introduces Scalafmt: #4965

makes it harder to backport changes to previous branches and it introduces conflicts to in-progress PRs

Is this a reason strong enough to continue using a deprecated language feature that we will have to remove anyway at some point when it will be completely dropped? Do you have any suggestions on how can we do better?

@ijuma
Copy link
Contributor

ijuma commented May 9, 2018

In this case, the feature itself is harmless and the benefit is small, so it's lower priority than some of the other things we have to do for the upcoming release.

For the scalafmt PR, we probably need to discuss it in the mailing list since it's even more disruptive and people have strong feelings when it comes to stuff like that.

@joan38
Copy link
Contributor Author

joan38 commented May 9, 2018

@ijuma I see, let's keep this PR open until I manage to add scalafix so that it will add more value like linting and automatic refactoring with other potential rules.

people have strong feelings when it comes to stuff like that

Indeed this is why Scalafmt would remove these feelings since there would be a rule settled for everyone. Personally I have no preference except consistency across the codebase and the peace of mind of not having to think about that because it will be taken care by a tool.
I will send a message to the mailing list then?

@joan38 joan38 changed the title Remove Scala prodecure syntax Remove Scala procedure syntax May 9, 2018
Copy link

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

@github-actions github-actions bot added the stale Stale PRs label Oct 21, 2024
Copy link

This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.

@github-actions github-actions bot added the closed-stale PRs that were closed due to inactivity label Nov 21, 2024
@github-actions github-actions bot closed this Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-stale PRs that were closed due to inactivity stale Stale PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants