LibreGaming/matchbot
LibreGaming
/
matchbot
15
1
Fork 1
Code Issues Pull Requests Projects Releases 3 Activity

Improve reliability and quality of the system namespace #1

Merged
seabass merged 4 commits from seabass/matchbot:system-improvements into master 2022-03-26 15:05:32 +01:00
Conversation 3 Commits 4 Files Changed 2 +12 -19
seabass commented 2022-03-23 00:10:28 +01:00
Owner
Copy Link

This pull request improves the system namespace, fixing a non-critical bug in the system's start/stop mechanism and removing unnecessary code.

Commits

Remove use of state container in main function

Since the main function will only ever start and stop one system, this commit simplifies the main function to use a single lexical binding for the system state rather than creating an Atom.

Remove unnecessary system/system function

A nil value acts as an empty map wherever a map value is expected. For this reason, this commit removes the system/system function (which returns an empty map), since a call to it can safely be replaced with a nil value.

Use a Var instead of an Atom for system state

The Atom's swap! function may run its provided function multiple times. Because of this, using an Atom for the system state can occasionally result in duplicate TCP connections being opened when the system is started. These connections remain open until they timeout (this is typically 5 minutes for IRC).

This commit changes the container for the system state from an Atom to a Var, whose alter-var-root function (analogous to swap!) does not have this issue.

This pull request improves the `system` namespace, fixing a non-critical bug in the system's start/stop mechanism and removing unnecessary code. # Commits ## [Remove use of state container in main function](https://git.libregaming.org/seabass/matchbot/commit/56094478dbe5398d6a8524e8bee5339ab814737e) Since the main function will only ever start and stop one system, this commit simplifies the main function to use a single lexical binding for the system state rather than creating an Atom. ## [Remove unnecessary system/system function](https://git.libregaming.org/seabass/matchbot/commit/fecd09c65624afef5565d7120421891a948a8643) A nil value acts as an empty map wherever a map value is expected. For this reason, this commit removes the system/system function (which returns an empty map), since a call to it can safely be replaced with a nil value. ## [Use a Var instead of an Atom for system state](https://git.libregaming.org/seabass/matchbot/commit/0abd793fe7b6bd83d62a7b3f690a22d549723b55) The Atom's swap! function may run its provided function multiple times. Because of this, using an Atom for the system state can occasionally result in duplicate TCP connections being opened when the system is started. These connections remain open until they timeout (this is typically 5 minutes for IRC). This commit changes the container for the system state from an Atom to a Var, whose alter-var-root function (analogous to swap!) does not have this issue.
seabass added 3 commits 2022-03-23 00:10:29 +01:00
56094478db Remove use of state container in main function 
Since the main function will only ever start and stop one system, this
commit simplifies the main function to use a single lexical binding for
the system state rather than creating an Atom.

Signed-off-by: Sebastian Crane <seabass-labrax@gmx.com>
0abd793fe7 Use a Var instead of an Atom for system state 
The Atom's swap! function may run its provided function multiple
times. Because of this, using an Atom for the system state can
occasionally result in duplicate TCP connections being opened when the
system is started. These connections remain open until they
timeout (this is typically 5 minutes for IRC).

This commit changes the container for the system state from an Atom to a
Var, whose alter-var-root function (analogous to swap!) does not have
this issue.

Signed-off-by: Sebastian Crane <seabass-labrax@gmx.com>
fecd09c656 Remove unnecessary system/system function 
A nil value acts as an empty map wherever a map value is expected. For
this reason, this commit removes the system/system function (which
returns an empty map), since a call to it can safely be replaced with a
nil value.

Signed-off-by: Sebastian Crane <seabass-labrax@gmx.com>
seabass requested review from sm2n 2022-03-23 00:10:53 +01:00
sm2n requested changes 2022-03-23 20:49:30 +01:00
Dismissed
src/system.clj Outdated
@ -39,4 +37,1 @@
:state nil
:irc nil})
(defn start [system]
sm2n commented 2022-03-23 20:18:29 +01:00
Owner
Copy Link

start doesn't actually use its input as far as I can tell? I think it'd be better to use _ as the binding name here since that is the case.

`start` doesn't actually use its input as far as I can tell? I think it'd be better to use _ as the binding name here since that is the case.
seabass commented 2022-03-23 20:54:44 +01:00
Author
Owner
Copy Link

I was imagining that, in the future, this could be used to pass custom options to a single instance for development purposes, and also eventually for the main function to read the config and pass it in rather than system/start doing it by itself. Do you have any preference here? For now, yes, I agree it would be better to use _ as the binding name.

I was imagining that, in the future, this could be used to pass custom options to a single instance for development purposes, and also eventually for the main function to read the config and pass it in rather than `system/start` doing it by itself. Do you have any preference here? For now, yes, I agree it would be better to use `_` as the binding name.
sm2n commented 2022-03-23 21:14:57 +01:00
Owner
Copy Link

No, I don't have a preference here. I think what you said makes sense with regards to custom options etc. Feel free to mark this resolved.

No, I don't have a preference here. I think what you said makes sense with regards to custom options etc. Feel free to mark this resolved.
seabass commented 2022-03-24 23:08:47 +01:00
Author
Owner
Copy Link

Resolved in commit 91cbdb69da.

Resolved in commit 91cbdb69da.
sm2n marked this conversation as resolved
src/system.clj Outdated
@ -61,2 +55,2 @@
(reset! system-atom (system))
(swap! system-atom start)))
(alter-var-root system-var stop)
(alter-var-root system-var (constantly nil))
sm2n commented 2022-03-23 20:19:27 +01:00
Owner
Copy Link

No reason for this either, I think? As the system value isn't used for start, and running stop multiple times is safe.

No reason for this either, I think? As the system value isn't used for `start`, and running `stop` multiple times is safe.
seabass commented 2022-03-24 23:10:00 +01:00
Author
Owner
Copy Link

I've just made commit 91cbdb69da - is this what you had in mind?

I've just made commit 91cbdb69da - is this what you had in mind?
sm2n marked this conversation as resolved
seabass added this to the 1.0.1 milestone 2022-03-23 20:56:32 +01:00
seabass added 1 commit 2022-03-24 23:06:08 +01:00
91cbdb69da Simplify the system start and restart functions 
This commit changes the binding name of system/start's argument to
_ (since it is currently not used within the function), and simplifies
the system/restart function by not swapping the system Var's content
with nil before filling it with the new-created system state.

Eventually, these changes will be reverted when the initialisation of
the system is separated from the starting of that system, which will
mean that system/start will read its argument and that system/restart
will have to reinitialise the system state before restarting it.

Signed-off-by: Sebastian Crane <seabass-labrax@gmx.com>
seabass commented 2022-03-24 23:08:02 +01:00
Author
Owner
Copy Link

Simplify the system start and restart functions

This commit changes the binding name of system/start's argument to _ (since it is currently not used within the function), and simplifies the system/restart function by not swapping the system Var's content with nil before filling it with the new-created system state.

Eventually, these changes will be reverted when the initialisation of the system is separated from the starting of that system, which will mean that system/start will read its argument and that system/restart will have to reinitialise the system state before restarting it.

## [Simplify the system start and restart functions](https://git.libregaming.org/LibreGaming/matchbot/commit/91cbdb69daf66e7077255fd69b206ef4d3ac4eab) This commit changes the binding name of system/start's argument to _ (since it is currently not used within the function), and simplifies the system/restart function by not swapping the system Var's content with nil before filling it with the new-created system state. Eventually, these changes will be reverted when the initialisation of the system is separated from the starting of that system, which will mean that system/start will read its argument and that system/restart will have to reinitialise the system state before restarting it.
seabass requested review from sm2n 2022-03-24 23:10:07 +01:00
sm2n commented 2022-03-26 00:42:33 +01:00
Owner
Copy Link

LGTM

LGTM
seabass removed review request for sm2n 2022-03-26 15:02:50 +01:00
seabass dismissed sm2n’s review 2022-03-26 15:03:35 +01:00
Reason:

Resolved

seabass commented 2022-03-26 15:04:46 +01:00
Author
Owner
Copy Link

Thank you for reviewing, @sm2n!

Thank you for reviewing, @sm2n!
seabass merged commit 91cbdb69da into master 2022-03-26 15:05:32 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
sm2n
Labels
Clear labels   
security

A verified or potential vulnerability

No Label
security
Milestone
Clear milestone
No items
No Milestone
1.0.1
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: LibreGaming/matchbot#1
No description provided.
Delete Branch "seabass/matchbot:system-improvements"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?