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

Created Trim Model Binder #300

Merged

Conversation

EfrainReyes
Copy link
Member

#299

En buena medida lo hice gracias a este post de stackoverflow: http://stackoverflow.com/a/1734025/2563028

sin embargo cuando escribí el test para verificar el trimming en el caso donde el parámetro que se pasa al action es un string simple, me estaba fallando, así que le hice override a BindModel también para poder hacer trim ahí.

Otra cosa que hice fue que cambié el nombre del namespace Core en el proyecto web EmpleoDotNet a EmpleoDotNet.Code para que no se confundiera con el proyecto EmpleoDotNet.Core

@@ -1,7 +1,7 @@
using System;
using System.Web;

namespace EmpleoDotNet.Core
namespace EmpleoDotNet.Code
Copy link
Member

Choose a reason for hiding this comment

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

que pasa que cambiaste este NS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lo expliqué en el texto.

Copy link
Member

Choose a reason for hiding this comment

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

@EfrainReyes en vez de hacer el cambio del namespace, hazle el cambio a proyecto EmpleoDotNet.Core a EmpleoDotNet.Domain Que fue lo que se acordo cuando se estuvo implementando el layer de app services. Creo que es un buen momento para hacerlo ya que esta causando conflictos.

Copy link
Member Author

Choose a reason for hiding this comment

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

Creo que eso está fuera de scope para este PR, y va a causar conflictos con los otros PR que están arriba. Eso debería ser un issue aparte.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Tienes razón. De mi parte eso era todo. +1 all pull request.

@jgtaveras
Copy link
Member

@EfrainReyes algo interesante que se pudieras considerar es solo agregar el model binder para los strings algo así como

ModelBinders.Binders.Add(typeof(string), new StringBinder());

por que, que pasa si luego necesitamos agregar un modelbinder a las fechas o a los números ?? tendríamos que modificar ese el TrimModelBinder o crear el binder por separado y luego agregarlo ??

@luis-ramirez
Copy link
Member

+1

luis-ramirez added a commit that referenced this pull request Mar 14, 2016
@luis-ramirez luis-ramirez merged commit b3886bc into developersdo:develop Mar 14, 2016
@EfrainReyes EfrainReyes deleted the feature/299-trim-model-binder branch March 17, 2016 04:46
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