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 conclave directory with README explaining this fork #2

Open
wants to merge 1 commit into
base: conclave/master
Choose a base branch
from

Conversation

shamsasari
Copy link

No description provided.

@ac101m
Copy link

ac101m commented Jul 18, 2022

Would it maybe make sense to keep the original readme and just have an additional one for conclave? Now that I think about it, maybe it makes sense to put all of our stuff in it's own directory within the repository.

@shamsasari
Copy link
Author

Would it maybe make sense to keep the original readme and just have an additional one for conclave?

The problem then becomes discovery. How does someone new learn how this repo works without being told to open a particular file?

Now that I think about it, maybe it makes sense to put all of our stuff in it's own directory within the repository.

That's the plan. All of the contents of the graal dir in the SDK would be moved to conclave here.

Copy link

@bon000 bon000 left a comment

Choose a reason for hiding this comment

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

Minor comments

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@shamsasari
Copy link
Author

The patch itself has been applied in a separate PR: #1

@shamsasari
Copy link
Author

"Upstream" seems to be standard way to refer to the repo that was forked: https://stackoverflow.com/questions/2739376/definition-of-downstream-and-upstream.

@shamsasari shamsasari mentioned this pull request Jul 18, 2022
@bon000
Copy link

bon000 commented Jul 18, 2022

"Upstream" seems to be standard way to refer to the repo that was forked: https://stackoverflow.com/questions/2739376/definition-of-downstream-and-upstream.
Never heard of that, not a blocker from my side, just wondering if this was confusing.
The fact that more than 1k people have voted for that question on Stack Overflow make me think that too many people have searched for that and maybe another terminology could have been used.

@shamsasari
Copy link
Author

Comments addressed.

@filipesoliveira
Copy link

filipesoliveira commented Jul 18, 2022

The patch itself has been applied in a separate PR: #1

I am not sure why we are doing that. What is the plan to merge graal to repository? Are you doing that as part of another PR?

@ac101m
Copy link

ac101m commented Jul 18, 2022

The problem then becomes discovery. How does someone new learn how this repo works without being told to open a particular file?

I'd suggest dropping a one-two sentence description at the top of the existing readme, and also a link to a readme in the conclave directory.

@shamsasari
Copy link
Author

I am not sure why we are doing that.

Purely to have a separate commit in the git history with just our changes.

What is the plan to merge graal to repository? Are you doing that as part of another PR?

I'm not sure what you mean here. But if you're referring to the graal dir in the SDK, then yes the plan is to move that here into a new conclave directory.

@shamsasari shamsasari closed this Jul 18, 2022
@shamsasari shamsasari deleted the shams-update-readme branch July 18, 2022 12:18
@shamsasari shamsasari restored the shams-update-readme branch July 18, 2022 12:18
@shamsasari shamsasari deleted the shams-update-readme branch July 18, 2022 12:18
@shamsasari shamsasari restored the shams-update-readme branch July 18, 2022 12:19
@shamsasari shamsasari reopened this Jul 18, 2022
@shamsasari
Copy link
Author

shamsasari commented Jul 18, 2022

@ac101m I like your suggestion. Done.

@shamsasari shamsasari changed the title Replaced the README with information of how this fork works @shamsasari Added conclave directory with README explaining this fork Jul 18, 2022
@shamsasari shamsasari changed the title @shamsasari Added conclave directory with README explaining this fork Added conclave directory with README explaining this fork Jul 18, 2022
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