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

Added VTCx places in Thailand #84

Closed
wants to merge 3 commits into from
Closed

Conversation

pepa65
Copy link

@pepa65 pepa65 commented Jul 9, 2020

Hope this contribution is OK as is. I'm happy to do more if I find time, but these have been long overdue.

Copy link
Contributor

@rbuj rbuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chiang Rai International Airport works fine on Weather Report applet, but I couldn't get the weather info for Chiang Rai and Mae Sariang.

$ xmllint --xpath "//code/text()" data/Locations.xml.in | sort | grep VTC.*
VTCC
VTCH
VTCL
VTCN
VTCP
VTCR
VTCS
VTCT
$ ./autogen.sh --prefix=/uar && make
$ xmllint --noout --dtdvalid data/locations.dtd data/Locations.xml 
$ grep "Chiang Rai" data/Locations.xml
$ sudo make install

MATE Weather Report panel applet:

Screenshot at 2020-07-09 21-09-34

Screenshot at 2020-07-09 21-09-57

https://aviationweather.gov/adds/dataserver_current/httpparam?dataSource=metars&requestType=retrieve&format=xml&hoursBeforeNow=3&mostRecent=true&stationString=VTCT&fields=raw_text

https://aviationweather.gov/adds/dataserver_current/httpparam?dataSource=metars&requestType=retrieve&format=xml&hoursBeforeNow=3&mostRecent=true&stationString=VTCR&fields=raw_text

https://aviationweather.gov/adds/dataserver_current/httpparam?dataSource=metars&requestType=retrieve&format=xml&hoursBeforeNow=3&mostRecent=true&stationString=VTCS&fields=raw_text

Copy link
Contributor

@rbuj rbuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rbuj rbuj requested a review from a team July 9, 2020 19:31
@pepa65
Copy link
Author

pepa65 commented Jul 9, 2020

Have added all locations in Thailand from sao.cty now.

@raveit65
Copy link
Member

Are all new location tested?
And does all new locations give us weather infos?

I don't think we should locations without weather infos.

Copy link
Member

@raveit65 raveit65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:

@pepa65
Copy link
Author

pepa65 commented Jul 17, 2020

OK, sorry about that, I was assuming everything in sao.cty would therefore give weather info. I don't have the infrastructure to test it. I tried building, but got stuck (see issue #86).

I have changed the pull request to only include the locations you have been able to confirm and the ones that are already recognized in the current applet.

I am very excited to be able to contribute, I spent many hours looking up the information and collating it.

@raveit65
Copy link
Member

Sorry, the location aren't confirmed by me. You have to do that.

@raveit65 raveit65 dismissed their stale review July 17, 2020 17:04

Changes aren't tested by author!

@pepa65
Copy link
Author

pepa65 commented Jul 17, 2020

Wait, I checked the locations in the current applet!

@raveit65
Copy link
Member

I tested your first list again

Chiang Rai     no
Chiang Rai International Airport   yes
Mae Sariang    no
Buriram        yes
Chaiyaphum     no
Chanthaburi    no
Chumphon       yes
Kanchanaburi   no
Koh Samui      yes
Krabi          yes
Loei           yes
Mae Sariang    no
Mae Sot        yes
Mukdahan       no
Nakhon Phanom  yes
Nakhon Ratchasima  yes
Nakhon Sawan   no
Nakhon Si Thammarat yes
Narathiwat     yes
Nong Khai      no
Pattani        no
Phetchabun     yes
Phitsanulok    yes
Phetchaburi    no
Prachinburi    no
Prachuap Khirikhan no
Roi Et         yes
Sakon Nakhon   no
Satun          no
Sakon Nakhon   no
Songkhla       no
Sukhothai      yes
Trat           no
Uttaradit      no

Not selectable in preferences!
+        <!-- A city in Thailand -->
+        <name>Tak</name>
+        <coordinates>16.876689 99.138131</coordinates>
+        <location>
+          <name>Tak</name>
+          <code>VTPT</code>
+          <coordinates>16.876689 99.138131</coordinates>
+        </location>

I am still wondering that you open a PR, but you're not able to rebuild libmateweather with your changes.
And that you think that other people should spend their spare time to do your work.
Donation welcome.
https://mate-desktop.org/donate/

@pepa65
Copy link
Author

pepa65 commented Jul 18, 2020

I don't have these lines in HEAD (52c2249):

+        <!-- A city in Thailand -->
+        <name>Tak</name>
+        <coordinates>16.876689 99.138131</coordinates>
+        <location>
+          <name>Tak</name>
+          <code>VTPT</code>
+          <coordinates>16.876689 99.138131</coordinates>
+        </location>

This PR is basically an xml source file, I've checked its schema validity, but indeed, I haven't been able yet to build the library... Tried, but got stuck, I might need some help. But I know others are able to build. This contribution is more on the content of the weather stations' locations. Sorry, I would love to be able to build it. I thought that this kind of non-technical contributions could be useful for the project, because apparently nobody that has been involved has supplied this information. I would be grateful if someone else could build this.

<name>Tak</name>
<coordinates>16.876689 99.138131</coordinates>
<location>
<name>Tak</name>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really, this is from you!!!

<!-- A city in Thailand -->
        <name>Tak</name>
        <coordinates>16.876689 99.138131</coordinates>
        <location>

@raveit65
Copy link
Member

raveit65 commented Jul 18, 2020

Is current state of PR what you want to get merged or is something missing?
Otherwise i will review it again....

@pepa65
Copy link
Author

pepa65 commented Jul 19, 2020

I have pushed the modified file every time after I received feedback from you, so the current state should automatically be updated every time (there is no longer an entry for "Tak").

@raveit65
Copy link
Member

I fixed more problems with 67521e5
And cherry-picked to 1.24.

@raveit65 raveit65 closed this Jul 19, 2020
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.

3 participants