Skip to content
This repository was archived by the owner on Jun 14, 2024. It is now read-only.

Conversation

@dmytroDragan
Copy link

@dmytroDragan dmytroDragan commented Apr 9, 2021

What is the context for this pull request?

What changes were proposed in this pull request?

Files (which were changed in #402) were missing scalafmt formatting.
As result PR-402 is hard to review due to mix of issue fix and scalafmt formatting.

To resolve it, the simplest approach is to apply scalafmt formatting in first place.
After merging this PR PR-402 should be unblocked.

Does this PR introduce any user-facing change?

No

How was this patch tested?

No need

@sezruby
Copy link
Collaborator

sezruby commented Apr 10, 2021

Could you confirm that this was from hyperspace/dev/.scalafmt.conf?

Copy link
Collaborator

@sezruby sezruby left a comment

Choose a reason for hiding this comment

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

Generally looks good to me (if it's formatted with hyperspace/dev/.scalafmt.conf), but I'm not sure about the rearrangement part. (maybe it's intelliJ default?) cc @imback82 do we have any arrangement rule?

@dmytroDragan
Copy link
Author

Hi @sezruby! It's formatted with hyperspace/dev/.scalafmt.conf

@sezruby
Copy link
Collaborator

sezruby commented Apr 14, 2021

@dmytroDragan could you revert changes from "rearrangement"?
I think the current arrangement looks better especially for IndexLogEntry, tag related functions / variable.
As we don't have a specific rule for arrangement for now, let's let that part so that we could merge this change quickly.

Copy link
Contributor

@andrei-ionescu andrei-ionescu left a comment

Choose a reason for hiding this comment

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

I did reviewing after 4-5 times where I did check how scalafmt reformatting is applied in my IDE and I can say that all these changes are not needed.

If we would add these changes, that it would mean that when I would apply reformatting on this file on my side, I'll be back to the original version with lots of formatting changes.

I recommend the you double check your formatting settings.

As I already posted on another of your PRs...

Here is my scalafmt setup in IntelliJ Idea (I have 1.5.1 version):

hyperspace_scalafmt

I usually do Command+Shift+Alt+L to reformat the file and use the following options in the dialog box:

hyperspace_reformat

If you use IntelliJ IDEA we can review other settings too so that you get it right. I've been through similar issues where applying formatting but after a few retries I managed to make it work and have as less possible changes into PRs due to formatting.

name.equals(that.name) &&
size.equals(that.size) &&
modifiedTime.equals(that.modifiedTime)
size.equals(that.size) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not scalafmt. I applied styling on the code and here it looks like this:

  override def equals(o: Any): Boolean = o match {
    case that: FileInfo =>
      name.equals(that.name) &&
      size.equals(that.size) &&
      modifiedTime.equals(that.modifiedTime)
    case _ => false
  }

No extra spaces.

Please check your scalafmt.

schemaString: String,
numBuckets: Int,
properties: Map[String, String])
case class Properties(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. No changes required. Here is how this part looks after I applied scalafmt reformat command:

object CoveringIndex {
  case class Properties(columns: Properties.Columns,
    schemaString: String,
    numBuckets: Int,
    properties: Map[String, String])

  object Properties {
    case class Columns(indexed: Seq[String], included: Seq[String])
  }
}

No changes.

case class Update(
appendedFiles: Option[Content] = None,
deletedFiles: Option[Content] = None)
case class Update(appendedFiles: Option[Content] = None, deletedFiles: Option[Content] = None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I don't have any changes.

Please check you formatting settings.

Content.fromLeafFiles(appended.map(toFileStatus), fileIdTracker),
deletedFiles =
Content.fromLeafFiles(deleted.map(toFileStatus), fileIdTracker)))))))))))
copy(source = source.copy(plan = source.plan.copy(properties = source.plan.properties.copy(
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither here - I don't have any changes after applying the reformatting.

@dmytroDragan
Copy link
Author

Th problem was with 1.5.1 vs 2.7.5 scalafmt.
#418

@dmytroDragan dmytroDragan deleted the issue400-just-scalafmt-formated branch April 14, 2021 12:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants