Add an XML Linter to the project

vinitamurthi commented 4 years ago

Our project has a lot of XML files that describe the look and feel of the application. We would like to add a linter that ensures all the XML files follow the same style. Xmllint seems to be a good fit for us and we would like to use that for our XML files. If you have recommendations for some other linter, please reach out to me to discuss if its a viable option before using it for the project. Steps that need to be completed to add the linter:

[ ] Run the linter on our existing XML files. Create a PR that fixes all lint problems that the linter catches
[ ] Add steps in the linter job of our CircleCI workflow (similar to the Kotlin linting step) that will install and run the linter on our XML files
[ ] Verify that the Android Studio auto-formatter is using the same rules as the linter configuration. If not, make changes to the code style of the XML files in Android Studio (that will create changes to the files present in the .idea/codeStyles/ directory). Create a PR that checks in the changes in these files.
[ ] Add documentation on how to install the linter in the prerequisites section of our wiki

NullByte08 commented 4 years ago

@vinitamurthi Can I work on the first task? I was thinking of using lint provided by Android studio itself.

vinitamurthi commented 4 years ago

Sure you can work on this. The Android Studio lint command works for every file right? I am worried that lint would end up affecting our kotlin/protobuf files as well

vinitamurthi commented 4 years ago

Another thing to keep in mind is that we need to run the linter on circleCI as well, which will not have android studio installed. Also avoid using gradle linting because we will be switching to bazel soon

NullByte08 commented 4 years ago

Sure you can work on this. The Android Studio lint command works for every file right? I am worried that lint would end up affecting our kotlin/protobuf files as well

We can manually set up the lint for particular files and exclude other files.

Another thing to keep in mind is that we need to run the linter on circleCI as well, which will not have android studio installed. Also avoid using gradle linting because we will be switching to bazel soon

lint is a standalone tool which can be installed using sdkmanager from command line and run from command line itself without having to install Android Studio or Gradle. But I am not at all familiar with circleCI.

vinitamurthi commented 4 years ago

Sure you can work on this. The Android Studio lint command works for every file right? I am worried that lint would end up affecting our kotlin/protobuf files as well

We can manually set up the lint for particular files and exclude other files.

Another thing to keep in mind is that we need to run the linter on circleCI as well, which will not have android studio installed. Also avoid using gradle linting because we will be switching to bazel soon

lint is a standalone tool which can be installed using sdkmanager from command line and run from command line itself without having to install Android Studio or Gradle. But I am not at all familiar with circleCI.

Okay, let's first verify if it will work as expected for CircleCI as well as on individual machines. One thing to consider is that different people would have different android SDK versions installed so it would be good to verify that the lint tool wouldn't get affected by that. Also, It may be a good idea to look into CircleCI and the configuration we have set up (You can see that here) @BenHenning what do you think about using the Android Studio lint tool for XML linting?

vinitamurthi commented 4 years ago

It would be a good idea to write this down into a small design doc, and we can review it before going forward!

vinitamurthi commented 4 years ago

@NullByte08 Any update on this issue?

NullByte08 commented 4 years ago

@vinitamurthi sorry I was busy in other stuff. If the style guidelines are set then I can run the lint according to that on the codebase and make a pull request to correct the lint errors. Or else, I will check the default lint constraints in android studio and inform you about those. About the circleCI, I have no experience in it so I will read some docs and tutorials and the link you provided to check how we can integrate the Android lint in it. Jenkins has this as a direct plugin but circleCI does not.

I ran the lint before and found these basic errors and warnings and typos:

NullByte08 commented 4 years ago

I ran the above only on the layout directory

vinitamurthi commented 4 years ago

Sounds good, if we can run it on CircleCI then I am totally open to using this. As far as the style guide goes, we have a guide written down here. Is that useful?

NullByte08 commented 4 years ago

@vinitamurthi Ok I will work on it and update you on the go.

NullByte08 commented 4 years ago

@vinitamurthi I am unable to take out time to work on the issue so I am currently unassigning myself from this issue. If I get enough time and nobody works on the issue I will work on this issue in the future.

anandwana001 commented 3 years ago

If anyone wants to check on xmllint how does it works:

pre:
    - sudo apt-get install libxml2-utils

anandwana001 commented 3 years ago

Keeping here for reference - https://github.com/alexjlockwood/android-lint-checks-demo

anandwana001 commented 3 years ago

@vinitamurthi Should this issue be blocked on this issue as per the comment - https://github.com/oppia/oppia-android/issues/1742#issue-688440933 ?

vinitamurthi commented 3 years ago

If you mean #1742 be blocked on this issue then yes I think it should be

anandwana001 commented 3 years ago

@vinitamurthi I am thinking opposite here, if we add Android Lint then we can directly use the lint for lining XML files so, in that way, this issue is blocked till the #1742 gets solved correct me if I misunderstood something here!!

BenHenning commented 3 years ago

I believe this is part of our static analyses GSoC project. @anandwana001 can you confirm?

anandwana001 commented 3 years ago

Yes, I guess.

Ensure all XML files follow our XML style guide.

I am bit worry on this here, as this is something which require Bazel support, and not be done using Gradle, is it ok under the project timeline? @BenHenning

BenHenning commented 3 years ago

That's fair @anandwana001. I think the main requirements that need to be met here are:

The XML file should be valid XML & using Android's XML namespaces
XML attributes should be ordered in a similar grouping order to Android Studio. One approach that will work: use lexicographical sorting except for "similar" items (specifically: margin, padding, layout width/height, constraint sets are important to group together)
Element top-level spacing for open & close tags should be 2 spaces and attribute spacing should be 4 spaces (+2 continuation from the previous line)
Nesting should introduce +2 spaces
Attributes should always be on their own line (and never on the same line as the open tag name) unless the whole open/close tag line fits within the 100 character limit
Attribute lines do not need to follow the 100 character limit since there's no way to wrap them
The opening < bracket for the opening tag should always be right next to the element tag name with no spaces, and never on its own line
The closing > bracket for the opening tag should always be right next to the attribute with no spaces, and never on its own line
The = symbol should always be right next to the attribute name & value without whitespace on either side
When /> is used (i.e. when the opening tag also serves as a closing tag) it should have 1 space before it and the end of the last attribute of the tag

Am I missing anything?

Correctly formatted example (from home_fragment.xml):

<?xml version="1.0" encoding="utf-8"?>
<layout
  xmlns:android="http://schemas.android.com/apk/res/android"
  xmlns:app="http://schemas.android.com/apk/res-auto">

  <data>

    <import type="org.oppia.android.R" />

    <variable
      name="viewModel"
      type="org.oppia.android.app.home.HomeViewModel" />
  </data>

  <FrameLayout
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:background="@color/white"
    android:gravity="center">

    <androidx.recyclerview.widget.RecyclerView
      android:id="@+id/home_recycler_view"
      android:layout_width="match_parent"
      android:layout_height="match_parent"
      android:clipToPadding="false"
      android:overScrollMode="never"
      android:paddingTop="36dp"
      android:paddingBottom="148dp"
      android:scrollbars="none"
      app:data="@{viewModel.homeItemViewModelListLiveData}" />

    <View
      android:layout_width="match_parent"
      android:layout_height="6dp"
      android:background="@drawable/toolbar_drop_shadow" />
  </FrameLayout>
</layout>

oppia / oppia-android

Add an XML Linter to the project #970