Skip to content

Conversation

@layomia
Copy link
Contributor

@layomia layomia commented Sep 5, 2019

Addresses #1412 (comment).
Used in dotnet/docs@3a082aa.

@layomia layomia requested review from mairaw and rpetrusha September 5, 2019 15:57
@layomia layomia self-assigned this Sep 5, 2019
@ahsonkhan
Copy link
Contributor

Why did we decide to call all the cs files Program.cs and move the name to a folder instead?

cc @mairaw, @rpetrusha

@rpetrusha rpetrusha added this to the September 2019 milestone Sep 5, 2019
@rpetrusha
Copy link

Having csharp files in a cs directory allows for multiple languages to share the same parent directory (for example, we can now add a vb directory for a vb example). It also allows the language of the example to be readily determined without having to look at file extensions, and is useful for CI tools.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thank you for making this change, @layomia. Are you planning to submit a PR that changes the path to the samples where they're used in the docs repo? Although I've approved, we should wait to merge until that PR is ready.

@rpetrusha rpetrusha added the 🚧 Hold for related PR Indicates a PR can only be merged when other related PRs are merged (see comments for links) label Sep 5, 2019
@ahsonkhan
Copy link
Contributor

ahsonkhan commented Sep 5, 2019

Having csharp files in a cs directory allows for multiple languages to share the same parent directory (for example, we can now add a vb directory for a vb example). It also allows the language of the example to be readily determined without having to look at file extensions, and is useful for CI tools.

I understand that we want to have language specific sub-directories.

However, this PR is changing the file names too:

snippets/standard/datetime/json/computing-with-jsondocument-error.cs → snippets/standard/datetime/json/computing-with-jsondocument-error/cs/Program.cs

How about something like the following (rather than calling them all Program.cs within a directory that contains the name)? We can still have the cs subdirectory.
snippets/standard/datetime/json/cs/computing-with-jsondocument-error.cs

Also, what about the existing samples? They look to be in the csharp directory:
https://github.com/dotnet/samples/tree/master/snippets/csharp/api
We also have:
https://github.com/dotnet/samples/tree/master/csharp

Why are we using the "standard" directory for these? I don't really understand the folder layout of this repo, tbh.

And the last time I added a sample, I ended up putting it in "core":
https://github.com/dotnet/samples/tree/master/core/json

See #492 (comment) from @BillWagner / @richlander

@layomia
Copy link
Contributor Author

layomia commented Sep 9, 2019

@rpetrusha

Are you planning to submit a PR that changes the path to the samples where they're used in the docs repo?

Yes, dotnet/docs#14056 changes the paths.

Can you please advise on the correct path structure, i.e.

snippets/standard/datetime/json/computing-with-jsondocument-error/cs/Program.cs (current), versus
snippets/standard/datetime/json/cs/computing-with-jsondocument-error.cs,

and also the correct directory to place these samples, re @ahsonkhan's comment: #1418 (comment).

@ahsonkhan
Copy link
Contributor

@rpetrusha - any updates on this? If it makes no difference, I am fine with the current changes as is. I'd like to get this merged.

Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

We can defer any further folder/file name restructuring, imo.

@mairaw
Copy link
Contributor

mairaw commented Sep 16, 2019

As I mentioned in the previous PR where we discussed this, I think the folder name should be csharp, so it matches the language identifier and other paths where we've been using this:

e.g.
https://github.com/dotnet/samples/tree/master/csharp
https://github.com/dotnet/samples/tree/master/snippets/csharp
https://github.com/dotnet/samples/tree/master/snippets/standard/base-types/format-strings/csharp

Thoughts?

@ahsonkhan
Copy link
Contributor

ahsonkhan commented Sep 17, 2019

Seems reasonable to me. For example, I would be fine with the following layout:
https://github.com/dotnet/samples/tree/master/snippets/standard/datetime/json/csharp/computing-with-jsondocument-error/Program.cs

https://github.com/dotnet/samples/tree/master/snippets/standard/datetime/json/csharp/datetime-converter-examples/example1/Program.cs
etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🚧 Hold for related PR Indicates a PR can only be merged when other related PRs are merged (see comments for links)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants