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

Setup some infrastructure for the repository. #60

Merged
merged 7 commits into from
Jun 1, 2019

Conversation

tannergooding
Copy link
Member

This sets up a lot of infrastructure for the repository including using Directory.Build.props/targets and adding various scripts for building, restoring, testing, and packing.

I believe the ultimate end-goal is to get on dotnet/arcade, but this should still be a step in the right direction.

It does not currently enable strong-name signing (as it needs to be determined if a custom or existing strong name key will be used). It would also be trivial to hook up authenticode signing once that is resolved.

If we have an existing AzDO repo, I can also add the azure-pipelines.yml file and get that enabled.

# suggest static readonly declarations be pascal case
# suggest type parameters be prefixed with T
###############################################################################
[*.cs]
Copy link
Member Author

Choose a reason for hiding this comment

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

These all match the VS defaults modulo csharp_indent_case_contents_when_block

csharp_indent_block_contents = true
csharp_indent_braces = false
csharp_indent_case_contents = true
csharp_indent_case_contents_when_block = false
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only new option that differs from the VS default. The VS default is true for compatibility reasons. This option is used in combination with csharp_indent_case_contents and you have effectively the following combinations:

(VS Default) csharp_indent_case_contents=true, csharp_indent_case_contents_when_block=true:

switch (value)
{
    case 1:
        break;

    case 2:
        {
            break;
        }
}

csharp_indent_case_contents=false, csharp_indent_case_contents_when_block=true:

switch (value)
{
    case 1:
    break;

    case 2:
        {
            break;
        }
}

csharp_indent_case_contents=false, csharp_indent_case_contents_when_block=false:

switch (value)
{
    case 1:
    break;

    case 2:
    {
        break;
    }
}

(The one I chose) csharp_indent_case_contents=true, csharp_indent_case_contents_when_block=false:

switch (value)
{
    case 1:
        break;

    case 2:
    {
        break;
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't like the corefx style, but since you've written most of the code here now you can pick whatever you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Which one of the four is your preference? I don't feel particularly strongly as long as the editorconfig ensures it is automatically normalized.

Copy link
Contributor

Choose a reason for hiding this comment

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

The last one seems ok. I don't know why it's not the default. The VS Default looks ugly.

Copy link
Member Author

Choose a reason for hiding this comment

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

The last one is the one I already choose for the repo (and was the one I thought you were indicating you didn't like) 😄

The first is the VS default for compatibility reasons at this point. The option allowing the last setup was only introduced a couple versions ago (I think VS2015 or 2017) and hasn't been around for very long at this point.

*.suo
*.user
*.sln.docstates
###############################################################################
Copy link
Member Author

Choose a reason for hiding this comment

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

The main .gitignore file has a lot more than necessary. I can keep it if desired, but I personally find the smaller subset easier to reason about.

Copy link
Member Author

Choose a reason for hiding this comment

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

(It also helps expose places where things aren't ending up where expected as almost all excluded files should end up under artifacts).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree ... I just let GitHub decide so this is better imo too.

@@ -0,0 +1,73 @@
<?xml version="1.0" encoding="utf-8"?>
<Project>
Copy link
Member Author

Choose a reason for hiding this comment

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

Centralized basically all of the relevant project settings here.

Directory.Build.props Outdated Show resolved Hide resolved
</ItemGroup>

<!-- Package versions for package references across all projects -->
<ItemGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

Package versions are centrally managed here.

This was referenced May 31, 2019
@mjsabby
Copy link
Contributor

mjsabby commented May 31, 2019

What do you mean by existing AzDO repo? Do we need a repo? I thought we just need an instance.

@tannergooding
Copy link
Member Author

tannergooding commented May 31, 2019

Right, I meant instance. Something like you have for the LLVMSharp repo.

@mjsabby
Copy link
Contributor

mjsabby commented May 31, 2019

Ok ... I'd love if it could be in dnceng .

@tannergooding
Copy link
Member Author

Ok ... I'd love if it could be in dnceng

I've pinged the e-mail thread to see what is recommended here.

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.

2 participants