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

Compose UI 24 code skeleton1 #69

Conversation

nick-polyak-ms
Copy link
Contributor

#47

@nick-polyak-ms nick-polyak-ms requested a review from a team July 22, 2022 20:00
@mgasca
Copy link

mgasca commented Jul 23, 2022

The pull request guidelines state that "We generally prefer squashed commits, unless multi-commits add clarity or are required for mixed copyright commits."

This PR has 17 commits that don't have good individual commit messages. A quick scan through them one by one doesn't provide improved clarity over the whole or mixed copyright. Can this PR please be rebased and pushed as a single commit?

I'm going to review it as is, not sure how github will deal with resolution of my comments if PR history is rewritten, I guess we'll see.

@mgasca
Copy link

mgasca commented Jul 23, 2022

It's not immediately clear from the structure and directory/project names what is under Tryouts and what is under Prototypes.

It is typical to supply readme.md files not just for the root of the repo, but also for distinct areas of cohesion. When we're talking about code, this can extend as far as having readme.md files for individual projects.

Can we please add readme files at least at the Prototypes and Tryouts level to communicate what is and should be under them?

Edit: As I start to look at the code I'm even more confused about these 2. I see projects under src/Common/Core project referencing things under Tryouts/Core.

@mgasca
Copy link

mgasca commented Jul 24, 2022

This PR is very large and doesn't seem to be targeted at adding one thing. This makes it more difficult to review. It's hard to know where to start and what route to take through the changes because it's not clear from the PR description or commit messages how everything included in the PR is related.

I commented earlier that the existing commits in this PR don't provide clarity. However, if it was broken up into separate commits that focused on specific areas that are being added here that may provide clarity.

@BalassaMarton
Copy link
Contributor

Why is there still a Tryouts and a Prototypes folder? Can you merge these? Having this in the main branch was a hard sell in itself - if you want to keep temporary code in the mainline, let's at least have it under a single folder, in a way how it would be if it was under src.

@nick-polyak-ms
Copy link
Contributor Author

The pull request guidelines state that "We generally prefer squashed commits, unless multi-commits add clarity or are required for mixed copyright commits."

This PR has 17 commits that don't have good individual commit messages. A quick scan through them one by one doesn't provide improved clarity over the whole or mixed copyright. Can this PR please be rebased and pushed as a single commit?

I'm going to review it as is, not sure how github will deal with resolution of my comments if PR history is rewritten, I guess we'll see.

I think there is an option to squash all commits before merging the pull request to Main or develop branches, which is what I intend to do. If there is no such option, I'll squash it when it is the only thing left in order to avoid repeated squashing.

@nick-polyak-ms
Copy link
Contributor Author

It's not immediately clear from the structure and directory/project names what is under Tryouts and what is under Prototypes.

It is typical to supply readme.md files not just for the root of the repo, but also for distinct areas of cohesion. When we're talking about code, this can extend as far as having readme.md files for individual projects.

Can we please add readme files at least at the Prototypes and Tryouts level to communicate what is and should be under them?

Edit: As I start to look at the code I'm even more confused about these 2. I see projects under src/Common/Core project referencing things under Tryouts/Core.

I'll add requested readme files and I'll check that src/common/core projects should not have any reference to the tryouts - definitely a mistake.

@nick-polyak-ms
Copy link
Contributor Author

This PR is very large and doesn't seem to be targeted at adding one thing. This makes it more difficult to review. It's hard to know where to start and what route to take through the changes because it's not clear from the PR description or commit messages how everything included in the PR is related.

I commented earlier that the existing commits in this PR don't provide clarity. However, if it was broken up into separate commits that focused on specific areas that are being added here that may provide clarity.

The larger parts of these PR are actually prototypes and samples which do not have to undergo the same scrutiny as the rest of the code. The code skeleton is actually have a very small number of files. But yes, I can split the prototypes away from the code skeleton for clarity sake.

@nick-polyak-ms
Copy link
Contributor Author

nick-polyak-ms commented Jul 25, 2022

Why is there still a Tryouts and a Prototypes folder? Can you merge these? Having this in the main branch was a hard sell in itself - if you want to keep temporary code in the mainline, let's at least have it under a single folder, in a way how it would be if it was under src.

Will do - I put Prototypes under Tryouts - merging them.

@nick-polyak-ms
Copy link
Contributor Author

Created a new pull request with smaller number of prototypes (based on the reviewer's wishes). closing this one

@nick-polyak-ms
Copy link
Contributor Author

Why is there still a Tryouts and a Prototypes folder? Can you merge these? Having this in the main branch was a hard sell in itself - if you want to keep temporary code in the mainline, let's at least have it under a single folder, in a way how it would be if it was under src.

merged

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