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

Refactor and clean-up #50

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Refactor and clean-up #50

wants to merge 9 commits into from

Conversation

pilif0
Copy link
Collaborator

@pilif0 pilif0 commented Jun 18, 2019

While I am going through the code, I will do my best to clean it up and make it more readable. This includes adding documentation and comments where useful, renaming variables to reflect content, replacing tabs with spaces, indenting, ...

@pilif0 pilif0 self-assigned this Jun 18, 2019
@PetrosPapapa
Copy link
Member

Hey @pilif0, I think using scalafmt to fix my horrible and inconsistent style would be much more productive and effective in the long term. The default configuration is supposed to be very good, but happy for you to suggest custom options.

@pilif0
Copy link
Collaborator Author

pilif0 commented Jun 24, 2019

@PetrosPapapa It would in the long term. I did this by hand, as I had to read the code to understand it anyway. I can add the configuration for that tool as part of this PR, so that it is available for use in the future.

@pilif0
Copy link
Collaborator Author

pilif0 commented Jun 24, 2019

I tried to configure scalafmt as best as I could. One issue I still have with the current configuration is with how it multilines long argument lists. For example:

  def this(store: PiInstanceStore[Int], l: PiProcess*)
          (implicit system: ActorSystem, context: ExecutionContext, timeout: FiniteDuration) =
    this(store,SimpleProcessStore(l :_*))

is formatted as

  def this(
      store: PiInstanceStore[Int],
      l: PiProcess*
  )(implicit system: ActorSystem, context: ExecutionContext, timeout: FiniteDuration) =
    this(store, SimpleProcessStore(l: _*))

which, in my opinion, is not an improvement. However, I don't know if there is a configuration option for this case, so we might have to live with this.

@pilif0 pilif0 marked this pull request as ready for review June 26, 2019 11:59
@PetrosPapapa PetrosPapapa changed the base branch from master to v1.4.0 June 27, 2019 00:23
@PetrosPapapa PetrosPapapa mentioned this pull request Jun 27, 2019
6 tasks
Base automatically changed from v1.4.0 to master July 9, 2020 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants