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

Introduce ObjectMap #1638

Merged
merged 11 commits into from
Feb 25, 2019
Merged

Introduce ObjectMap #1638

merged 11 commits into from
Feb 25, 2019

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Feb 21, 2019

Add ObjectMap class
Move ResourceTree class into separate file, rename as HttpResourceTree
Rename HttpServer::resourceTree as paths, make public
Deprecate HttpServer::addPath and HttpServer::setDefault methods

Addresses #1637

@slaff slaff added this to the 3.7.2 milestone Feb 22, 2019
// Check existing connection
connection = httpConnectionPool.valueAt(i);
if(connection->getConnectionState() > eTCS_Connecting && !connection->isActive()) {
debug_d("Removing stale connection: State: %d, Active: %d", connection->getConnectionState(),
connection->isActive());
delete connection;
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 wonder, is it necessary to delete the connection? Could we just reset it, perhaps?


void setDefaultHandler(const HttpPathDelegate& callback);
void setDefaultResource(HttpResource* resource);
void addPath(const String& path, HttpResource* resource)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe deprecate some of these, use resourceTree directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good idea. Deprecate it now and the removal will be after two or three major releases. The Readme.md has to be updated too.

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've renamed resourceTree to paths as it seems to make more sense to me when in use. I left the internal references (in HttpServerConnection) alone.

* }
*
*/
void set(const K& key, V* value)
Copy link
Contributor

Choose a reason for hiding this comment

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

That set will cause confusion. We have two ways to add things to the map that behave diffently:

ObjectMap<String, MyType> map;
map["key1"] = object1;
// and 
map.set("key1, object1);

The best would have been to make map["key1"] = object1; behave as set. But probably it would be better to remove completely V& operator[](const K& key) since I cannot think of a proper way to implement the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best would have been to make map["key1"] = object1; behave as set

Done :-)

Rename `ResourceTree` to `HttpResourceTree`
Deprecate `HttpServeri::addPath()` and `setDefault` methods
Make `HttpCompatResource` private, move into `HttpResourceTree` module
bodyParser = (*bodyParsers)[types.at(i)];
const String& type = types[i];
if(bodyParsers->contains(type)) {
bodyParser = (*bodyParsers)[type];
Copy link
Contributor Author

@mikee47 mikee47 Feb 23, 2019

Choose a reason for hiding this comment

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

bodyParsers->get(type) would be nicer... That's HashMap though...

/** @brief Access map entry by reference
* @param key
* @retval Value Guarded access to mapped value corresponding to given key
* @note If the given key does not exist in the map then it will be created and a null value entry returned.
Copy link
Contributor Author

@mikee47 mikee47 Feb 23, 2019

Choose a reason for hiding this comment

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

To avoid this (usually) un-desirable side-effect we could take advantage of our Value class and use it to create an entry in the map only when a value is actually assigned.

…by `operator[]`

`Value` revised to work with map directly rather than just taking value reference - safer and more useful
Move example code to class comment
@slaff slaff removed the 3 - Review label Feb 25, 2019
@slaff slaff merged commit 9079572 into SmingHub:develop Feb 25, 2019
@mikee47 mikee47 deleted the feature/ObjectMap branch February 25, 2019 11:02
mikee47 added a commit to mikee47/Sming that referenced this pull request Feb 25, 2019
mikee47 added a commit to mikee47/Sming that referenced this pull request Feb 25, 2019
In `HttpConnection::multipartProducer()` the `IDataSourceStream*` entry from `files` must be extracted, not removed
Add `extractAt()` method added to `ObjectMap` to support this operation more efficiently
mikee47 added a commit to mikee47/Sming that referenced this pull request Feb 25, 2019
In `HttpConnection::multipartProducer()` the `IDataSourceStream*` entry from `files` must be extracted, not removed
Add `extractAt()` method added to `ObjectMap` to support this operation more efficiently
slaff pushed a commit that referenced this pull request Feb 25, 2019
In `HttpConnection::multipartProducer()` the `IDataSourceStream*` entry from `files` must be extracted, not removed
Add `extractAt()` method added to `ObjectMap` to support this operation more efficiently
mikee47 added a commit to mikee47/Sming that referenced this pull request Feb 25, 2019
@slaff slaff mentioned this pull request Feb 27, 2019
4 tasks
mikee47 added a commit to mikee47/Sming that referenced this pull request Feb 27, 2019
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.

2 participants