Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Short folder names #1555

Merged
merged 1 commit into from
Jul 28, 2017
Merged

Short folder names #1555

merged 1 commit into from
Jul 28, 2017

Conversation

natemcmaster
Copy link
Contributor

As long as we are rearranging out entire codebase...

https://github.com/aspnet/KestrelHttpServer/tree/namc/folders

image

image

@pakrym
Copy link
Contributor

pakrym commented Mar 24, 2017

I think if we should do it only if it's going to be done for entire asp.net core. Also not before @halter73 change gets merged.

@natemcmaster
Copy link
Contributor Author

To be clear, there are no plans to do it for all of aspnet core, but I don't think that should stop us from using a cleaner layout. There is at least one other repo that has already done this.

Btw, this is on top off @halter73's change.

@cesarblum
Copy link
Contributor

Farewell GitHub history 😝

@cesarblum
Copy link
Contributor

Not entirely opposed to the change but let's first get the new packages' names right (*.Transport.*)

@natemcmaster
Copy link
Contributor Author

So, this?

- src/
  + Kestrel
  + Kestrel.Core
  + Kestrel.Transport.Libuv
  + Kestrel.Https

In the future we can add Kestrel.Transport.(Whatever).

Farewell GitHub history

Yeah :-/. We're gonna lose it no matter what in #1551. If only there was some way to follow git renames. :trollface:

@davidfowl
Copy link
Member

I'm going to say no to this since it's inconsistent I also prefer the long names. Get the github expander plugin.

@davidfowl
Copy link
Member

I also adds no value and history ends up being messed up for no good reason (and yes, GitHub should fix their CSS 🤣)

@davidfowl
Copy link
Member

davidfowl commented Mar 25, 2017

I also agree with @pakrym , if we're not doing this for all of ASP.NET Core it's pointless. Consistency is good across repos and EF for tend to be a bit different with some things done in the repo.

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Do not like

@natemcmaster natemcmaster deleted the namc/folders branch March 27, 2017 16:42
@natemcmaster natemcmaster restored the namc/folders branch July 12, 2017 19:28
@natemcmaster natemcmaster changed the base branch from halter73/828 to dev July 12, 2017 19:29
@natemcmaster
Copy link
Contributor Author

@davidfowl has had a change of heart.

@pakrym
Copy link
Contributor

pakrym commented Jul 12, 2017

Are we doing it for all repos?

@Tratcher
Copy link
Member

This kind of structure should be consistent across repos. All or none...

I can only assume this breaks some tooling regexes..

@natemcmaster
Copy link
Contributor Author

FYI all - details begin discussed internally, but the primary motivation is continual problems with filenames that grow beyond max path issues. Not sure yet if all repos will get this treatment or not, but regardless, I want this for Kestrel as this project has particularly long filenames, eg src/Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions/Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.csproj

Copy link
Contributor

@cesarblum cesarblum left a comment

Choose a reason for hiding this comment

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

Nice 👍

@halter73 halter73 dismissed davidfowl’s stale review July 12, 2017 21:36

This was review from months ago and without explanation.

@natemcmaster natemcmaster requested a review from davidfowl July 13, 2017 19:55
@natemcmaster
Copy link
Contributor Author

⌚ waiting until the work on rel/2.0.0 is done. This would make it painful to merge changes in rel/2.0.0 back to dev.

Remove the Microsoft.AspNetCore.Server prefix from csproj and their folders. This is required to help us avoid max path issues on Windows.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants