Skip to content
This repository was archived by the owner on Apr 2, 2020. It is now read-only.

A new function, normalize and a test case #5

Closed
wants to merge 8 commits into from

Conversation

idleman
Copy link
Contributor

@idleman idleman commented Apr 11, 2012

I have written a new function. It does resolve a given path and remove any "../". Can you check it out and notify all others it (if needed) and then tell me how should I "pull" next time? I am brand new to this I don´t know how it works exactly, it took me some while to get everything working.

PS. I saw there already existed encode/decode methods for pro-cent coding, and therefore did I not add it, even of course they should be handled in the function as well, but I want to hear what your thought are first.

If I don´t get any respond in some days will I assume this message never arrived and therefore repost it.

Thanks in advance.

//idleman

@idleman
Copy link
Contributor Author

idleman commented Apr 18, 2012

Well, what do you believe now? I used simple strings because the function require the possibility to insert values, so I did´t see any chance to use a iterator range. The method should also handle wide characters as well, but to be honest so do I really not know if it does, if it should really do any different.

template <typename Type>
inline std::basic_string<Type> normalize(std::basic_string<Type> path)
{
//why pass by value? See http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're not modifying the parameter, no need to pass by value. There is also no reason why this should be a "regular" function that takes a value and returns a new object. You might as well take the string as a non-cost lvalue reference and modify it in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it will make calls like: normalize("something") impossible. Why not just create a wrapper function in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly the point: you shouldn't allow normalize("something") when you obviously really want to modify a string using your normalization routine. What you should be supporting is:

template <class T> void normalize(basic_string<T> &path);

If anybody wants to use this function, they should do:

std::string path = "./foo";
normalize(path);
assert(path == "foo");

Also, this functionality sounds like it's already in Boost.Filesystem and is very specific to filesystem paths that I'm having a hard time seeing whether it's worth having in cpp-netlib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I see, in that case so should this pull request be closed, if you are not interested to have it. I just thought it could be a good thing to have in the library, especially when it remove any attempts to receive a directory above "wwwroot". But I came up with another thing, sending files in a effective way. See what I have written about it in "uri thread" on the Google groups thread. You can answer there as well, or maybe Glyn.

PS. I will let Glyn close this pull request, when you have decided if this function should be added or not, but if so, please let me know faster, so I don´t waste time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've replied to that thread. As far as this pull request is concerned I think it's better if you look at whether Boost.Filesystem does what you want to do before you abandon this work. If it doesn't then it might be worth adding this functionality to that library because this is about filesystem paths already and not URIs.

About not wasting time, the best way to avoid this is to let us know what you want to work on and communicate with us how you intend to do it or while you're doing it. Thanks for the effort and I'm looking forward to your other contributions in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been busy, therefore late answer

Well, I currently only use the async_server and the uri class, so anything which make those classes better would I be interested in, specially in async_server implementation. Its robustness, exception safety, the possibility to add more "logging possibilities", performance and so on. Maybe a short web-server example would be a great thing to-do as well, because I already comfortable with async_server impl :-) But in that way, were should I write all questions? Github, google groups?

Copy link
Contributor

Choose a reason for hiding this comment

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

The best place to let everyone know what you want to work on is the mailing list. There's no other place where the discussions happen but there.

Github issues are reserved for filing bugs. General support and development concerns are discussed in the mailing list (Google groups).

As for the web server example, look at https://github.com/cpp-netlib/cpp-netlib/blob/master/libs/network/example/http/fileserver.cpp which already does practically everything you'd want from a fileserver. If you want to extend that to do other things then be my guest and send a pull request against it.

Let's continue this discussion in the mailing list to get a larger audience involved.

divyekapoor pushed a commit to divyekapoor/cpp-netlib that referenced this pull request May 19, 2012
@glynos glynos closed this Jan 29, 2013
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.

3 participants