"Colon Spacing" rule name update #3583
Conversation
|
The CocoaPods pipeline failed on an unrelated issue. I think it might be transitory (how do I re-run the pipelines) The |
ghost
mentioned this pull request
|
Hello @jpsim, This PR has been sitting unreviewed for almost 3 weeks. Can you please review or suggest someone who I can talk to to get this progressed? |
|
Hello @otaviocc, @fredpi, @agarmash, @cltnschlosser, Sorry for the direct message. Does anyone know who the maintainers of SwiftLint are? I filed this PR and its corresponding issue about a month ago and it still hasn't progressed. Can any of you review or suggest who I could talk to? (btw, if you're wondering why I chose to ping you, I just looked at who contributed to SwiftLint in 2021) |
There was a problem hiding this comment.
Hi @raduciobanu002, thank you for the contribution! Sorry that this has been sitting around for so long. I understand that can be frustrating.
To answer your question about maintainers: @jpsim is the maintainer of the project, and the rest of us try to contribute when we get time. But obviously it's a completely voluntary effort from all parties, so sometimes PRs can get stuck for a while.
As for the change itself, I think it seems reasonable. Just to confirm: This only changes how the name of the rule is displayed, correct? If folks are referencing the colon rule in their config, this won't break anything?
Yeah the identifier is used in config. I believe name is just used for display. |
Thanks! That's what I thought, but having confirmation from someone else is helpful. I'm going to go ahead and merge this since it seems like an innocuous change and has been stuck for a while. @jpsim if you have any issues, please let me know, we can always revert. |
|
Hmm seems like Buildkite is failing because it can't build the |
Thanks @sethfri. I appreciate this is totally voluntary and am grateful for your assistance. TBH I didn't mind waiting as long as I know the PR will get looked at at some point. As mentioned in my message I simply didn't know the structure of the maintaining team 🤷
Yes, to the best of my knowledge of the project this PR should only change the display name of the rule and not anything else. The Danger warnings are because it's testing the warning display names specifically. Thanks again @sethfri and @cltnschlosser for looking at this. |
|
So, after having reverted all the changes to match I'm out of ideas. |
Don't feel bad! There are several things happening here that make this pretty confusing. The tl;dr is that you need to sync your fork. If you're curious why, I did some spelunking and I'm happy to walk you through it. I don't know your level of experience, so to avoid back and forth and help you as best I can the first time, I'm going to index towards over-explaining. Apologies if anything comes across as patronizing. Notice in the build output that Buildkite says it's If you take a look at SwiftLint's latest Gemfile.lock, you'll notice we're using ffi 1.15.0, which of course is more recent than 1.12.2. That's our next clue. The next question I asked was: What changed between ffi 1.12.2 and 1.15.0? So I went over to their repo. Their CHANGELOG doesn't mention anything related to our issue that I can see, so I went to their release list. You can see 1.12.2 was published in Feb 2020, and 1.15.0 was published in March 2021, so that's the time range we're interested in. I went all the way back to Feb 2020 in their commit history and started looking for a commit that looked relevant. That's when I found ffi/ffi#746, which deals directly with the missing method we're seeing! They made ffi no longer use So, the fix is to make sure your branch is using the same ffi 1.15.0 that the I hope that makes sense! Please let me know if you have any questions, or need help syncing your fork. I'd expect the Buildkite issue to stop once you do that, and we can finally get this merged. |
|
@sethfri Ah... I didn't take into account that I'm working with multiple Remotes 😊 Sorry, I'm still a bit new to contributing to open source projects. However, another thing that contributed to this being a problem is that buildkite tried to build my branch rather than pulling into With your advice I can see the build is now successful. I am grateful for your thorough explanation and I'm sorry you've had to spend this much time investigating and then doing the write-up. Thanks again! |
Most CI I've seen for PRs typically just builds against the PR branch. I agree with you it'd probably be better for it to pull your change in and build against
No need to apologize. It was fun and doesn't take that much time when you know what you're looking for. The steps become internalized 😊 Plus, it's always nice to help someone level up.
I hope you'll continue doing so! We can always use more contributors. Sorry again for the headaches on this PR, and thanks for your contribution! |
Why?
The way
SwiftLintwas assembling some strings resulted in phrasing that... wasn't great:How?