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

Ew move readme to special files #1705

Closed

Conversation

EWright212
Copy link
Contributor

No description provided.

@pablobm
Copy link
Collaborator

pablobm commented Jul 11, 2020

@EWright212, this appears to be the same as #1698, but with two additional commits. Is this an accident?

@EWright212
Copy link
Contributor Author

@EWright212, this appears to be the same as #1698, but with two additional commits. Is this an accident?

Hi @pablobm! No, this is a new branch to solve a different issue (being that the README has a table in it that doesn't look nice on GitHub). However it is based on the logic from the previous commit which broke symlinks, as discussed with @nickcharlton.

@pablobm
Copy link
Collaborator

pablobm commented Jul 14, 2020

Oh, I see. The problem here is that the PR becomes difficult to review, because it comes with a number of unrelated changes. This happens sometimes, and it can be a bit tricky. Here are a couple of options:

  1. Wait for the other PR to be merged, then create this one on top of the new master.
  2. Create the PR on top of ew-fix-windows-symlink-issue instead of on master. If you click to edit the PR, you'll see this option as a dropdown under the title. See this screenshot from a different PR:

Option to change the branch that a PR refers to

Option 2 is probably what I would do here, but it may still involve rebasing things later when the first PR is merged, and changing this back to master. It's not perfect, but at least is gives us better visibility to review your work for now.

@nickcharlton
Copy link
Member

I don't think we can do that as it's from a fork 👀

Let's get the original merged and then come back to this, as that'll make life a bit easier.

@nickcharlton
Copy link
Member

As we've merged #1698, do you think we could rebase this?

@nickcharlton
Copy link
Member

I don't seem to be able to force push this branch now I've rebased it, so I just opened #1776.

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.

None yet

3 participants