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

Range validation enhancement #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

notzippy
Copy link

@notzippy notzippy commented Jan 9, 2015

Reference revel/revel#760

@brendensoares
Copy link
Member

@AnonX @pushrax next step is to merge this and then refine it until we have all the details we need to decide if we want to implement it or not. Sound right?

@notzippy
Copy link
Author

notzippy commented Mar 4, 2015

If it takes over 2 months for an RFC to get merged I can see people not even bothering. It would be far quicker to simply have written the change and done a PR against revel.

@ghost
Copy link

ghost commented Mar 9, 2015

@notzippy This requires too much mental work. Revel is far from release and agile like workflow would fit better than USSR style bureaucracy IMO.

As for the validators. Currently they do not work with int64, right? What if I need it?

@ghost
Copy link

ghost commented Mar 9, 2015

@brendensoares I said this before, let's allow Revel to evolve naturally. PRs from community are more important than attempts to meet expectations of those who complain about like "Revel's not following Golang way" and stuff like that. We should not care about those who would not use the framework anyway.

So, let's identify our own Revel way. What is it about? Productivity, scalability, low learning curve? Something else? And work to make these aspects of framework stronger. We do not need to moduralize the codebase ASAP. There are many users who do not care about that aspect. What do you think, @pedromorgan, @notzippy? We can go on developing everything by evolution rather than revolution.

And when we have enough info and clear vision we can even rewrite some components from scratch (if we feel like it). But only when we have clear vision and we really need it. Not just to meet some abstract golang way.

Conclusion: let's focus on bringing invaluable tool to those developers for whom Destination is more important than a Journey, for those who need to solve their everyday tasks rather than meditate on the idea of how enlightened and smart they are so they can use the minimal 100% modular 0 overhead 1 bit less memory consumption fully golang way solution (there are many other Go frameworks / toolkits for them).

@pedromorgan
Copy link
Member

@AnonX Yes.. feels like this project is stalling.. needs more release often.. with minor increments..

Master is way outta scope compared to develop.. so next change is a major "break" imho

@brendensoares
Copy link
Member

@AnonX I understand your perspective. RFC's are intended to make Revel safe from bad decisions, not to make Revel bureaucratic. We still accept PRs for obvious bugs and minor changes.

Quoting from this repo's README.md:

Many changes, including bug fixes and documentation improvements can be implemented and reviewed via the normal GitHub pull request workflow.

Some changes though are "substantial", and we ask that these be put through a bit of a design process and produce a consensus among the Revel core team.

The "RFC" (request for comments) process is intended to provide a consistent and controlled path for new features to enter the framework.

That said, I think this RFC repo and workflow are going to be very beneficial.

@notzippy merging should happen very quickly. Refining and completing the RFC will take a bit longer to reach consensus. I haven't merged this yet, because I was hoping for some input from you and @AnonX and anyone else who wants to chime in to help define a workflow. No one actually answered my question:

@AnonX @pushrax next step is to merge this and then refine it until we have all the details we need to decide if we want to implement it or not. Sound right?

So I'll merge it now so we can make forward progress. We'll refine the workflow as we go.

@brendensoares
Copy link
Member

Looking at the README.md further, the process that we've adopted is already defined. You've already done a lot of the tasks.

TODO

  • Fork the RFC repo http://github.com/revel/rfcs
  • Copy 0000-template.md to active/0000-my-feature.md (where 'my-feature' is descriptive. don't assign an RFC number yet).
  • Fill in the RFC
  • Submit a pull request. The pull request is the time to get review of the design from the core team and the community.
  • Build consensus and integrate feedback. RFCs that have broad support are much more likely to make progress than those that don't receive any comments.
  • Eventually, somebody on the [core team] will either accept the RFC by merging the pull request and assigning the RFC a number, at which point the RFC is 'active', or reject it by closing the pull request.

Now it's time to build consensus and refine this PR.

@notzippy
Copy link
Author

I believe @AnonX opinion was this was missing the ability to handle int64 objects,

int64 Range: -9223372036854775808 through 9223372036854775807.
float64 is the set of all IEEE-754 64-bit floating-point numbers.

A float can only carry 15 significant digits and an int can carry 19 significant digits. This leaves a discrepancy between the two so checking an int64 using a float checker with 0 precision will not be accurate with numbers over 999,999,999,999,999 or less then -999,999,999,999,999 .

@brendensoares
Copy link
Member

We should discuss this at our weekly core team meeting.

@notzippy when did @AnonX mention this concern?

@ghost
Copy link

ghost commented Mar 23, 2015

@brendensoares 14 days ago here: #1 (comment)

@brendensoares
Copy link
Member

@AnonX I see.

As for the validators. Currently they do not work with int64, right? What if I need it?

So let's include int64 in this RFC, too.

@brendensoares
Copy link
Member

@notzippy we came to the conclusion on our team meeting that this feature really doesn't require an RFC. Agreed? We should just make the change. Right?

@notzippy
Copy link
Author

SGTM 👍

@brendensoares
Copy link
Member

@notzippy do we have a PR for this though?

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.

3 participants