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

Added HyphenNameMapperNoDigits - it maps e.g. "implS3" to "impl-s3" #140

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

amorfis
Copy link

@amorfis amorfis commented Feb 9, 2021

I use ficus and at work, we often have config classes like

case class MyConfig(impl: StorageImpl, local: Option[LocalStorageConfig], s3: Option[S3StorageConfig])

and we use hyphens in configs. So this maps to

impl = "S3"
s-3 = {
...
}

It would look much better if it was s3 instead of s-3

This PR makes it possible

@coveralls
Copy link

coveralls commented Feb 9, 2021

Coverage Status

Coverage increased (+0.05%) to 91.925% when pulling 712fd43 on amorfis:hyphen-no-numbers into f14bd7c on iheartradio:master.

@amorfis
Copy link
Author

amorfis commented Sep 3, 2021

Is anyone looking at this PRs? Or is the project dead? I need the change in ficus, but I don't want to fork another project of it if there is still one alive.

@kailuowang
Copy link

kailuowang commented Sep 3, 2021 via email

@amorfis
Copy link
Author

amorfis commented Sep 3, 2021

@kailuowang I have many projects and I don't want to copy code. I can of course add this to some utils library and use in my projects. Or I can release my own version of ficus. I thought it might be useful also for others. However, I fully respect your opinion if it is different.

Thanks for the ficus library!

@kailuowang
Copy link

@amorfis thanks!
Right now the PR still needs a scalafmt but I think once that's fixed I can merge and release.

Going forward though, full transparency, I am not using ficus myself. Will you be interested in help maintaining this library?

@amorfis
Copy link
Author

amorfis commented Sep 5, 2021

@amorfis thanks!
Right now the PR still needs a scalafmt but I think once that's fixed I can merge and release.

Going forward though, full transparency, I am not using ficus myself. Will you be interested in help maintaining this library?

Yes!

@amorfis
Copy link
Author

amorfis commented Sep 6, 2021

Is it possible to check why scalafmt test doesn't pass? When I run scalafmtCheckAll on my machine there are no errors at all.

@kailuowang
Copy link

Is it possible to check why scalafmt test doesn't pass? When I run scalafmtCheckAll on my machine there are no errors at all.

Cc @mdedetrich who set it up

@mdedetrich
Copy link

So I actually cloned https://github.com/amorfis/ficus/tree/hyphen-no-numbers and manually downloaded the scalafmt-native-musl release from https://github.com/scalameta/scalafmt/releases/tag/v2.7.5 and when I run scalafmt locally it passed without any problems.

I have no idea why the github action is failing since its also just downloading the same binary from the release and running it, there might be some weird git interaction I am not aware of? You can try dropping the Formatting commit and instead doing a rebase against https://github.com/iheartradio/ficus master, then running scalafmt + git commit ammend and then force pushing to see if it works.

Alternately @kailuowang can just merge this PR ignoring the failure.

@amorfis
Copy link
Author

amorfis commented Sep 7, 2021

Alternately @kailuowang can just merge this PR ignoring the failure.

But then the check is going to fail on master isn't it? And on all the future PRs.

@kailuowang
Copy link

kailuowang commented Sep 7, 2021 via email

@mdedetrich
Copy link

mdedetrich commented Sep 7, 2021

TBH I am not sure what this problem is because I have already integrated this precise solution in around 10 other Scala open source projects and this is the first time such a problem has happened.

Rationally speaking it leaves 2 possibilities

  1. Simply put the contents of the git clone in the CI machine for w/e reason are different (this is why I suggested doing the rebase)
  2. There is some weird error with this specific version of scalafmt

Something specifically being wrong with scalafmt-native looks highly unlikely, both the sbt runner and native are built from the exact same codebase. When I checked out @amorfis branch locally I also tried scalafmt via sbt cli and it reported the exact same result as scalafmt-native. In other words irrespective of what type of scalafmt I was using (native or sbt) I am unable to reproduce whats going on

I would prefer to diagnose this problem rather than ripping everything out, because I doubt that just using sbt scalafmt is an actual fix. If you want we can verify this, in which case @amorfis if you are open to this, can you temporarily add

WorkflowStep.Sbt(List("scalafmtCheckAll"), name = Some("check format project"))

After this line https://github.com/iheartradio/ficus/blob/master/build.sbt#L11

Then make sure you run sbt githubWorkflowGenerate, this will update the https://github.com/iheartradio/ficus/blob/master/.github/workflows/ci.yml file.

Commit these changes (ideally as an ammend to your c3f020b commit with a force push) and lets see what happens.

@amorfis
Copy link
Author

amorfis commented Sep 7, 2021

Will do tomorrow as I gotta go already today 👍

@amorfis amorfis force-pushed the hyphen-no-numbers branch 2 times, most recently from ee2dd0e to fd863d0 Compare September 9, 2021 06:36
@amorfis
Copy link
Author

amorfis commented Sep 9, 2021

I rebased on master and looks like there are no errors now, but it also looks like actions did not run?

@kailuowang
Copy link

Closing and reopen to trigger GH action

@kailuowang kailuowang closed this Sep 9, 2021
@kailuowang kailuowang reopened this Sep 9, 2021
@mdedetrich
Copy link

Awesome, it seems that scalafmt is working without problems now. Not sure what happened but it appears that rebase solve the issue.

@kailuowang kailuowang merged commit 537fd6f into iheartradio:master Sep 9, 2021
@amorfis
Copy link
Author

amorfis commented Sep 9, 2021

Osm! Thanks!

Do you plan to release new version anytime soon?

@mdedetrich
Copy link

If #166 can get merged before the release that would be great!

@amorfis
Copy link
Author

amorfis commented Sep 9, 2021

I'd love to merge #139 too

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.

4 participants