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

WIP: Pkellner built-in TH #2698

Closed
wants to merge 1 commit into from
Closed

Conversation

Rick-Anderson
Copy link
Contributor

@Rick-Anderson Rick-Anderson commented Feb 9, 2017

replaces #2644

@Rick-Anderson
Copy link
Contributor Author

@pkellner - impossible to rebase and fix the TOC so I created a new branch. Make sure you follow the format/style of Working with Forms

cc/ @guardrex

@Rick-Anderson Rick-Anderson changed the title Pkellner built-in TH WIP: Pkellner built-in TH Feb 9, 2017
@spboyer
Copy link
Contributor

spboyer commented Feb 9, 2017

This covers #1948 as well correct. Appears the content is there. Please be sure to set Fixes #1948 in PR.

@spboyer spboyer mentioned this pull request Feb 9, 2017
@pkellner
Copy link
Contributor

pkellner commented Feb 9, 2017

@spboyer There is overlap we need to resolve. I've mostly completed the first 3 built in tag helpers and working with @Rick-Anderson to get them properly committed and reviewed. After that, @guardrex @Rick-Anderson and I need to discuss how to best integrate what Luke is/was working on with distributed cache TH to make it consistent with the builtin TH doc's I'm doing. Not sure if it should be a separate article because it goes beyond basic "TH Defined" type doc or something else. I'll stay on top of it one way or another (that is #1948). Stay tuned...

@guardrex
Copy link
Collaborator

guardrex commented Feb 9, 2017

When I did the outline, I thought that each TH would get its own sample. It might be possible to have one sample cover all (or most) of the TH's in one shot 🔫 (perhaps with a few alternate Startups required). Otherwise, I think you covered everything in that outline. The <distributed-cache> TH is a different doc, so those bits aren't relevant (yet).

@pkellner
Copy link
Contributor

pkellner commented Feb 9, 2017

@guardrex I've not completely thought through the example aspect of the built-in TH docs I'm working through. I am keeping a Visual Studio project with all my sample code in a very organized way so it could become the "example" but because of the nature of TH's (each being so simple and discrete) I'm not sure they need a sample project. Might be worth taking offline and hashing out and then putting summary notes/suggestions here. The Distributed TH is a different beast because of the nature of, well, "distributed". That opens up a world of options. I think all the other tag helpers are more straight forward. I was thinking for my 4th TH I should work on Distributed because we are all thinking about it.

@Rick-Anderson
Copy link
Contributor Author

distributed cache TH will be it's own doc

@guardrex
Copy link
Collaborator

guardrex commented Feb 9, 2017

each being so simple and discrete

Yeah ... and if the goal isn't to have a ton of doc stubs lying around for months, then it makes sense to NOT put samples in for the first round of these. Could always come back later and add sample(s).

@pkellner
Copy link
Contributor

pkellner commented Feb 9, 2017

@Rick-Anderson I've now got this PR (2698) on my local checked out and it looks to me like it has all the latest changes I made. @guardrex made another 10 or so suggestions I'd like to incorporate before you review the three built in TH's here. I'm assuming I should just make changes and then commit to this branch? I don't see the TOC entry on top of any of the docs. Am I right in assuming you will do that? Luke gave a format for it I could follow if you'd rather me do it.

@guardrex
Copy link
Collaborator

guardrex commented Feb 9, 2017

The TOC is a doc tho. What I posted was the metadata that goes at the top of a doc. if surf around the repo, you'll see the TOC docs.

@pkellner
Copy link
Contributor

pkellner commented Feb 10, 2017

Sorry, but lost again in GitHub Pull Request. I have my changes ready on my local branch ra-pKellner-built-in-TH that I've pushed to my fork of docs (https://github.com/pkellner/Docs). When I try to create the pull request with github (see picture below), it looks like it wants to change everything. Any clue how I have gotten lost here? I would have expected the pull request would be small since I pulled that branch so recently to my local, made changes locally, checked those in and pushed to my repo.

Hoping maybe @guardrex can help?

My Attempted Pull Request

@pkellner
Copy link
Contributor

I've reset my local fork and am going through and making a pass of changes. I will have those checked in later today. I'm removing the "warning" I have about CacheItemPriority.NeverRemove because based on the source code here: https://github.com/aspnet/Caching/blob/dev/src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs I don't think that cache will ever be removed. If I'm wrong, please LMK @guardrex or @Rick-Anderson

@Rick-Anderson
Copy link
Contributor Author

[!WARNING]
The priority attribute does not guarantee a specific level of cache retention. CacheItemPriority is only a suggestion to the Cache Provider. Setting this attribute to NeverRemove does not guarantee that the cache will not be evicted. CacheItemPriority sets the priority the cache evicts items under memory pressure.

@JunTaoLuo can you confirm the following?

Setting this attribute to NeverRemove does not guarantee that the cache will not be evicted.

@JunTaoLuo
Copy link
Contributor

For the current FTS and LTS releases, setting the attribute to NeverRemove guarantees that the memory pressure triggered compaction will not evict that entry. It may still be evicted due to expiration or expiration tokens. Note that this behaviour is likely to change, see aspnet/Caching#221.

Rick-Anderson pushed a commit that referenced this pull request Jul 8, 2017
* Put in more reasonable cache times for examples.

* Added to ImageTagHelper.md a block on the top and guessed at updating assetid by incrementing 1.  Renamed all Additional Resources to Resources per @Devguard, fixed incorrect expires-sliding code example, removed warning about Cache NeverRemove.

* fixed blank lines below toc entry, made consistent "Additional resources", put back in warning about NeverRemove based on feedback from team: #2698 (comment)

* clean up

* created Distributed and Environment Tag Helper Doc Pages and worked on environment.

* Reworked sections from @Rick-Anderson previous commit and removed review comments.

* Updated Anchor Tag Helper's definition of asp-action and asp-controller to be more explicit about defaults as well as say "href" instead of "final URL".

* I realized that asp-all-route-data also behaves like asp-route-* with routing parameters so I updated the doc to reflect that.

* Updated var-by-route definition following comments from Damien in old github post aspnet/Mvc#1552

* Cleanup of Tag Helper Docs

* changed author to pkellner from Peter Kellner per @mairaw

* Update AnchorTagHelper.md

* Update CacheTagHelper.md

* Minor tweaks
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.

6 participants