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 a row in Synopsis #325

Closed
wants to merge 7 commits into from

Conversation

@SunveerSingh
Copy link

commented Nov 8, 2018

According to https://trac.osgeo.org/postgis/ticket/2767 a raw in Synopsis need to be added about AddRasterConstraints tablename and fieldname

Added a row in Synopsis
According to https://trac.osgeo.org/postgis/ticket/2767 a raw in Synopsis need to be added about AddRasterConstraints tablename and fieldname
@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

@SunveerSingh since this change is going to get applied to already released branches, could you also add a line about it to NEWS file in the root directory of repo, stating ticket number, change description and your name?

SunveerSingh added a commit to SunveerSingh/postgis that referenced this pull request Nov 8, 2018

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

@Komzpa Done, PR Sent

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

@Komzpa kan you run the travis again, I am having some network problem, that is why it failed.

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

Tests failed:

psql:/src/postgis/regress/00-regress-install/share/contrib/postgis/raster_comments.sql:4: ERROR:  function addrasterconstraints(name, name) does not exist

So, there is no such simple function. Sorry for misguidance. (It's good to have tests that check documentation :) )

Looking at https://github.com/postgis/postgis/blob/svn-trunk/raster/rt_pg/rtpostgis.sql.in#L8303 it's in fact a longer call that sets most of arguments by default. Can you go over all the AddRasterConstraints definitions in file https://github.com/postgis/postgis/blob/svn-trunk/raster/rt_pg/rtpostgis.sql.in and see whether they have a matching entry in documentation?

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

@SunveerSingh the one on line 8303 is the one that can be called as (tablename,fieldname), just that it has more optional parameters.

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

And I guess tablename and fieldname is not required so that is why the user "can" use, and as you said they are optional so we can instead of writing it here we can add a note "The user can call AddRasterConstraints tablename, fieldname.(Optional)"

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

@SunveerSingh "Note:" is never a good choice. :)

Can you look at XML near the ones you constructed? there is something with "opt" (that means parameter is optional), and some of the names have =true on them (meaning that when it's not set, it supposed to be True). So you need to add the tail of optional arguments to the block in your PR that will match the order and default values of the one on line 8303.

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

ok so first should I add tablename name; and fieldname name; here: https://github.com/postgis/postgis/blob/svn-trunk/raster/rt_pg/rtpostgis.sql.in#L8303?

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

@SunveerSingh they are already there. Names in code and in docs don't match, 'rasttable' is the same thing as 'tablename', and 'fieldname' is the same thing as 'rastcolumn'. If you dislike that they don't match you can go greater length and change that too, but that can be done after we're done with this change as separate PR :)

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

if they are already then what should I do now?

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

The way I see it:

  1. remove entry you added;
  2. on entry on line 447 in xml file, mark things that are optional and their default value according to order of things in L8303;
  3. add NEWS file change you did in #326 into this PR, #325.
@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

  1. remove entry you added-done!
@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

2.on entry on line 447 in xml file, mark things that are optional and their default value according to order of things in L8303;-Done!

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

@SunveerSingh in b197918 you added opt qualifier, that's right - but not default value (=true part, or maybe it's sometimes =false?). add that too please! :)

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

Done!

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

3.add NEWS file change you did in #326 into this PR, #325.-Done!

NEWS Outdated Show resolved Hide resolved
doc/reference_raster.xml Outdated Show resolved Hide resolved
doc/reference_raster.xml Show resolved Hide resolved

Komzpa and others added some commits Nov 8, 2018

Update NEWS
Co-Authored-By: SunveerSingh <33540878+SunveerSingh@users.noreply.github.com>
Update doc/reference_raster.xml
Co-Authored-By: SunveerSingh <33540878+SunveerSingh@users.noreply.github.com>
@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

What is left now?

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

Let's get a green tick from travis to see that we've not mixed it all up again, and we're probably good to go :)

@Komzpa

Komzpa approved these changes Nov 8, 2018

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

Thanks a lot! can we close this: https://trac.osgeo.org/postgis/ticket/2767#comment:6

@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

@SunveerSingh patience! a green tick from Travis, a merge of PR into release branch, and it will get closed. :)

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

No Worries, Thank a lot for helping me out!

@strk strk closed this in 73f8752 Nov 8, 2018

strk pushed a commit that referenced this pull request Nov 8, 2018

Documentation for AddRasterConstraint optional parameters
Patch by Sunveer Singh

Closes #2767
Closes #325


git-svn-id: http://svn.osgeo.org/postgis/branches/2.5@16997 b70326c6-7e19-0410-871a-916f4a2858ee
@Komzpa

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

@SunveerSingh congratulations, merged :)

@SunveerSingh

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

@Komzpa Thanks a lot! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.