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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract a component manager class #2427

Merged
merged 9 commits into from Jan 12, 2018

Conversation

@deivid-rodriguez
Copy link
Contributor

commented Dec 22, 2017

馃帺 What? Why?

This PR extracts some unrelated refactorings from my docker branch.

  • It removes the docker_development_app task. This generated application doesn't really work for development since it's not updated when changing the local code! Since a whole revamp is coming, I say let's remove it.

  • It extracts a ComponentManager class from common tasks around managing components. Maybe this could be the initial seed for a lerna for Ruby 馃槂. NOTE: These class "infers" the components from the decidim- prefix in folder names. This approach broke when adding the design application. I renamed it to decidim_app-design which I think it also makes it more clear that it's not a component but a full application.

馃搶 Related Issues

None.

馃搵 Subtasks

None.

馃摲 Screenshots (optional)

None.

馃懟 GIF

hidden

@ghost ghost added the in-progress label Dec 22, 2017
@codecov

This comment has been minimized.

Copy link

commented Dec 22, 2017

Codecov Report

Merging #2427 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2427   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files        1296     1296           
  Lines       30279    30279           
=======================================
  Hits        29883    29883           
  Misses        396      396
@josepjaume

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2017

Super nice!! Not entirely sold about the design app name, though. What about just design_app? Or even design?

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2017

I started with that name but it felt weird to have design-app as the package name in package.json (inside the folder). No big deal probably, maybe that file is not even used, and the "package" is not certainly meant to be published and only makes sense in the context of the decidim repo, right?

@deivid-rodriguez deivid-rodriguez force-pushed the chore/extract_component_manager branch from 71db158 to 7a3a2eb Dec 23, 2017
# frozen_string_literal: true

module Decidim
class ComponentManager

This comment has been minimized.

Copy link
@mrcasals

mrcasals Dec 26, 2017

Contributor

Docs?

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Dec 26, 2017

Author Contributor

Thanks @mrcasals, I'll add some comments. We seem to be blindly requiring all class definitions to be commented, should we enable

decidim/.rubocop.yml

Lines 802 to 804 in 80508ca

Style/Documentation:
Enabled: false
?

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Dec 26, 2017

Author Contributor

Happy fiestas, by the way! 馃槃

This comment has been minimized.

Copy link
@mrcasals

mrcasals Jan 8, 2018

Contributor

@deivid-rodriguez sorry for the late reply. I like having class comments to explain the purpose of the class. Sometimes it's a dull comment (for example, most of the controllers), but for other classes I find it useful to have a little context on why use that class and how to do it, so I would not disable this check.

Hope you had some happy fiestas you too!

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Jan 8, 2018

Author Contributor

Maybe we can enable the check with folder exceptions such as controllers/.

This generator was not working as expected since it would use th
decidim's code inside codegram's image, so development changes would not
be picked app by the development application.
Otherwise running things like `rake test_app test_all` fails.
This is not an engine/component, so component manager tasks fail on it.
Renaming also helps clarifying this.
@deivid-rodriguez deivid-rodriguez force-pushed the chore/extract_component_manager branch from 7a3a2eb to eaefdd9 Jan 9, 2018
@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2018

@mrcasals I added some comments over the class definition. I also tried to enable the Style/Documentation cop, and exceptions apart, it led to ~50 offenses. Not too bad.

@mrcasals

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

@deivid-rodriguez damn, it might call for some other PR then. Let's leave it as it is.

@josepjaume josepjaume merged commit 0a61d03 into master Jan 12, 2018
19 checks passed
19 checks passed
ci/circleci: accountability Your tests passed on CircleCI!
Details
ci/circleci: admin Your tests passed on CircleCI!
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: assemblies Your tests passed on CircleCI!
Details
ci/circleci: budgets Your tests passed on CircleCI!
Details
ci/circleci: build_test_app Your tests passed on CircleCI!
Details
ci/circleci: comments Your tests passed on CircleCI!
Details
ci/circleci: core Your tests passed on CircleCI!
Details
ci/circleci: main Your tests passed on CircleCI!
Details
ci/circleci: meetings Your tests passed on CircleCI!
Details
ci/circleci: pages Your tests passed on CircleCI!
Details
ci/circleci: processes Your tests passed on CircleCI!
Details
ci/circleci: proposals Your tests passed on CircleCI!
Details
ci/circleci: surveys Your tests passed on CircleCI!
Details
ci/circleci: system Your tests passed on CircleCI!
Details
ci/circleci: verifications Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codecov/patch Coverage not affected when comparing accc329...eaefdd9
Details
codecov/project 98.69% remains the same compared to accc329
Details
@ghost ghost removed the in-progress label Jan 12, 2018
@josepjaume josepjaume deleted the chore/extract_component_manager branch Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.