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

Removed copyright notices #1765

Merged
merged 1 commit into from
Jan 13, 2019
Merged

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Jan 13, 2019

Also made cosmetic improvements to code.

Blocks rust-lang/rust#57520.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 13, 2019

@Mark-Simulacrum Do you have permissions to merge this, so we can proceed with rust-lang/rust#57520 right after?

@Mark-Simulacrum Mark-Simulacrum merged commit c2c7c50 into rust-lang:master Jan 13, 2019
@carols10cents
Copy link
Member

Hey yinz, this isn't a super big deal because nothing yinz changed is actually critical or anything, but I'm a little annoyed at how this PR went down for the following reasons:

  • I would have preferred that the copyright notice removals happened in at least a separate commit from the other cosmetic improvements, and if the copyright notice removals were the actual urgent part, I would have preferred a separate PR for the urgent changes vs. the not urgent changes
  • I'm not sure why the copyright removals are so urgent that they needed to happen within hours of this PR being opened on the weekend, when steve and I didn't really have a chance to even help out here
  • Like, I wouldn't want to have gotten an urgent ping to take care of this, so in some sense I'm glad you took care of this Mark, and I don't want to discourage you from taking initiative like this if it's necessary, but I'm not convinced that it was really necessary in this case... y'know?
  • I don't want to get super territorial about code, but... neither of you have ever committed anything to this repo? So it was surprising to come back and see this change having been made?

Again, there's nothing wrong with this actual change, nothing is broken or ruined or anything, and I realize this is probably a one-time thing that won't happen again, but I just wanted to register my disappointment with the procedure here. Thanks ❤️

@alexreg
Copy link
Contributor Author

alexreg commented Jan 13, 2019

Sorry for the surprise.

First thing: who's "yinz"? I guess that message was basically for both of us though. :-) Anyway, bullet point one is fair. Not something I'm personally bothered about, but I can see why you or others might be, so will keep in mind. And yes, it's not like the changes were super-urgent, but they were blocking another PR on the Rust repo (to do with tidy), so it was nice not to hold them up... turns out there is another thing (broken intra-book links in this repo) holding it up, but oh well. I don't want to speak too much for Mark, but I (and probably he) thought the changes were sufficiently non-controversial there wouldn't be any problem. Also, was hoping this would save you the hassle and let you worry about more important things, but I know that's a fine line. Anyway, if you want to be in the process for all changes big and small, that's obviously your prerogative. :-)

P.S. Did you get my ping on my main PR? Will definitely need to involve you in that, though it should be quite simple I hope.

@carols10cents
Copy link
Member

Yinz is like y'all but better ;)

Lots of PRs on the Rust repo are held up for lots of reasons, but typically unless there's a really good reason, we follow processes. I prefer to follow usual processes even in times when it's not as important so that everyone has practice and knows the right processes to follow when it IS super important.

The fact that these changes are non-controversial is why I'm only annoyed, not angry :) The tricky bit is that when you're unfamiliar with a repo, it's hard to be 100% sure that changes are actually non-controversial. Neither of you stated explicitly in this PR that you had considered whether these changes were ok or not, and it's only because I trust Mark that I knew he did consider that before clicking merge (I don't know you well enough to trust you). It left me wondering, if you two hadn't thought about what was being changed, what I might come back to in the future.

I will take a look at the main PR tomorrow. I'm not going to drop everything for this though, because I still don't see the urgency in removing and linting for copyright notices.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 14, 2019

Ah, interesting dialectal word that! I learn something new. :-)

Anyway, fair enough... I had no idea of the usual process for the book repo. I'm a compiler dev, of course, and I just submitted this PR to to make things work with my PR there (okay, plus the cosmetic things).

Anyway, it was Mark's idea (or maybe @Centril's?) to add a copyright lint to tidy, so I'm just the guy implementing it. It's all part of the wider effort to remove them everywhere from the codebase (it's already been finished for the rust repo). I certainly don't want to put the blame on him (I think he acted pretty reasonably), but from my point of view I just thought it was okay to defer to anyone with merge rights here... oh well, live and learn.

And yes, take your time. I'm confident you'll agree things are okay once you get a chance to review this, but if not, any reversion should be easy too!

P.S. The issue I was referring to was that on master HEAD, there seem to be some broken links within the book (and I'm not sure why). That needs to be resolved before doing a submodule update, but let me know how you want to go about this first; I won't do anything for now.

@steveklabnik
Copy link
Member

Hey folks; I just wanted to chime in here that I had the same feels as @carols10cents , and it only took me so long to reply here because well, she said it better than I already :) I'm glad this worked out, but it does also make me a bit nervous.

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