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

Refactor/get all spaces listener #113

Closed
wants to merge 4 commits into from
Closed

Refactor/get all spaces listener #113

wants to merge 4 commits into from

Conversation

fboechats
Copy link
Contributor

refactor: factor out getAllSpaces listener

Factor out the getAllSpaces listener to its own file.

Note: I added a "All" in the name to be more intuitive the difference between this listener and the getSpace listener.

refs #104

Adding getSpace to listeners/index.js
Factor out the getAllSpaces listener to its own file.

refs #104
@juancarlosfarah
Copy link
Member

juancarlosfarah commented Jun 21, 2019

@fboechats, can you rebase and resolve the conflicts, please. There have been some recent changes that were merged after your PR.

Also, could you rename the method and files to getSpaces (plural)? This is how we are handling the difference in this repo. This is mainly to match the types and channels, which are called GET_SPACES.... Also, because getAllSpaces implies that we are getting all of the possible spaces. Most likely this is a pageable, which means that you can pass some params and get a given limit of spaces after a skip and a sort.

Thanks!

@juancarlosfarah
Copy link
Member

@fboechats, thanks for fixing this. Could you please squash your commits into one? That way the commit history stays clean.

Here is how to do it in case you haven't done it before (subtitle: The hard(er) and less flexible way):

https://gist.github.com/patik/b8a9dc5cd356f9f6f980#the-harder-and-less-flexible-way

The commit message can then be something like:

refactor: factor out getSpaces listener

Factor out the getSpaces listener to its own file.

refs #104

Thanks!

@fboechats
Copy link
Contributor Author

Can I create another PR?? I had a problem with the fork and lost connection with this PR.

@juancarlosfarah
Copy link
Member

Hi @fboechats, yes of course! You can close this PR and create a new one with the same changes and the correct commit.

@fboechats fboechats closed this Jun 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