search

lemberg / connfa-android

Open Source Android app for Conferences and Events
http://connfa.com
Apache License 2.0
100 stars 111 forks source link

Mass cleanup && refactoring, improve project, and much more 👌 #11

Closed rock3r closed 7 years ago

rock3r commented 7 years ago

This PR contains all the cleanup and prepping work done so far on our fork. It does not add or remove functionality and there is no user-facing change, it's all about under-the-hood maintenance.

rostyslawbulych commented 7 years ago

Hi @rock3r, thank you for the PR! We'll work on it next week.

rostyslawbulych commented 7 years ago

Hi @rock3r,

We've got a couple comments/questions:

  1. Libraries dependencies were moved out of gradle file. What benefits do you see from such approach?

  2. All properties were moved to the external, gradle-managed file. Such location seems to be a bit confusing to a new users. We would like to keep code as independent from concrete build-system as possible, we wouldn't like build-script to manage API keys and other properties.

  3. Fabric doesn't seem to deal with your updated scheme: only hard-coded in manifest key seems to be applied by system.

  4. Ideally, project should be compilable with no changes, so we have to put some compilable properties but not just sample files to the team-props.

  5. You have updated loadImageForFloor method of FloorPlansManager in order to fix some bug. Could you specify which bug was fixed with listener usage instead of pullFromServer method return?

  6. Regarding HURLCookieStore. In fact, HURLCookieStore is HTTP client sub-module, updating default cookie storage behavior. So it isn't just doing client's job - it's part of the client.

Everything else is clear.

Could you please check and provide your feedback.

Thanks!

rock3r commented 7 years ago

Hey @rostyslawbulych, sure. Most of the questions you're asking aren't really tied to this PR as they were done/addressed in the previous PR, but I'll reply here anyway.

  1. They were moved out of the main gradle file but they're still in a gradle file. This makes it easier to find out the dependencies when they're applied across modules, and makes sure all modules use the same version of a dependency
  2. If anything, it's more coherent now than it was originally, because now all properties are in the same folder and files, as opposed to being scattered across different files (gradle build files, resources, manifest, and java code). It took a considerable amount of time for us to find all the hardcoded values across the codebase and replace them with our values; now it's a matter of doing it all in one place. Will write some quick info about how to set up the project in the readme; the main difference is that, if you have all the required info, it takes 1 minute to fill it in across the project now as opposed to half an hour it took to us.
  3. Fabric works just fine on our side, you might have placed in the wrong values, or maybe they're in the wrong place? Please double check as all these changes are tested on our side as working already
  4. "Ideally, project should be compilable with no changes" I very much disagree with this. On the main branch, the project should not be tied to any specific configuration. If you really want to publish your API keys/secrets and signing configuration, you should have that on a branch that is specific to the event the configuration is relevant for. (I personally would rather not publish any secrets on a public repo)
  5. The only changes in that file are about code formatting (spacing and newlines) and about not using the application's static context. I don't see any other changes, not sure what you're asking about.
  6. I'm not sure what the question is? It seems like you're asserting something. If you're asking about the deprecation, that's because as mentioned in our call we want to move the networking layer to using Retrofit. Retrofit uses OkHttp, which does all the cookies management by itself. So we should really not use the HURLCookieStore in any new places, and if anything, reduce its usage as much as possible in anticipation of not needing it anymore. The fact is the app is using a deprecated HTTP client in the first place (Apache HTTP Client), which will go away sooner rather than later too.

Hope this answers all questions!

rostyslawbulych commented 7 years ago

Hi @rock3r, thanks for detailed feedback. We'll merge this PR into master today.

rock3r commented 7 years ago

Great, thanks :)