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

Refactor README.md #54

Merged
merged 23 commits into from
Feb 8, 2023
Merged

Refactor README.md #54

merged 23 commits into from
Feb 8, 2023

Conversation

mkgrgis
Copy link
Contributor

@mkgrgis mkgrgis commented Jan 2, 2023

By https://github.com/ibarwick/firebird_fdw/blob/master/README.md example of advanced FDW documentation

@mkgrgis mkgrgis mentioned this pull request Jan 4, 2023
@t-kataym
Copy link
Contributor

Thank you for the pull request.
I found there are 2 types of update in the pull request.

  1. One is the addition of useful information. It is grateful and we would like to take it in.
  2. Another is the movement of description. The updated format is inconsistent with the other modules in our PGSpider project. We need to consider what format should be for our project.

Therefore, we would like to apply the 1st one.
Could you create a pull request for it?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 17, 2023

The updated format is inconsistent with the other modules in our PGSpider project. We need to consider what format should be for our project.

@t-kataym, what about my PRs with refactoring of this files step-by-step ?

https://github.com/pgspider/jdbc_fdw/blob/main/README.md
https://github.com/pgspider/influxdb_fdw/blob/master/README.md
https://github.com/pgspider/dynamodb_fdw/blob/main/README.md
https://github.com/pgspider/parquet_s3_fdw/blob/main/README.md
https://github.com/pgspider/griddb_fdw/blob/master/README.md

https://github.com/EnterpriseDB/mongo_fdw/blob/master/README.md
https://github.com/EnterpriseDB/mysql_fdw/blob/master/README.md

What about merging of this PR after it will be completed other PRs?

@t-kataym
Copy link
Contributor

Thank you for your proposal.
We need to consider what is ideal format because we may get another Pull Request in the future.

Is there standard format as Readme of FDW?

Could you tell me the reason why you referred firebird_fdw as an example?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 18, 2023

Is there standard format as Readme of FDW?

I think no. There is only more or less detailed READMEs. For ex. mysql_fdw and oracle_fdw is very popular, but only README by @laurenz for the last is well-structured.

Could you tell me the reason why you referred firebird_fdw as an example?

  1. Order of topics look like natural : Features (do we need?), Supported platforms (cat it run on?), Installation (how to install?), Usage (quick start), Functions (additional features), technical details (Identifier case handling, Generated columns, Character set handling) etc., Limitations, ... Development roadmap, Useful links.
  2. README of firebird_fdw is more detailed than other (except https://github.com/laurenz/oracle_fdw/blob/master/README.oracle_fdw ) and completed.

@t-kataym
Copy link
Contributor

Thank you for your response.

What about merging of this PR after it will be completed other PRs?
Yes, I agree with you.

Following modules are original of PGSpider project (or upstream project is no longer maintained).

Could you create PRs of above FDWs?
Once they are completed, then I would like to merge this PR.

Following modules are forked from upstream project.
If the upstream is updated, it will be taken in our project.
I hope you to try to create PRs for upstream projects.

PGSpider project Upstream
https://github.com/pgspider/odbc_fdw https://github.com/CartoDB/odbc_fdw
https://github.com/pgspider/oracle_fdw https://github.com/laurenz/oracle_fdw
https://github.com/pgspider/parquet_s3_fdw https://github.com/adjust/parquet_fdw
https://github.com/pgspider/mongo_fdw https://github.com/EnterpriseDB/mongo_fdw
https://github.com/pgspider/mysql_fdw https://github.com/EnterpriseDB/mysql_fdw

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 20, 2023

pgspider/jdbc_fdw#14 done.

https://github.com/pgspider/pgspider_ext is an independent extension, not a foreign data wrapper, but

an extension to construct High-Performance SQL Cluster Engine for distributed big data

fdw documentation example isn't applicable to https://github.com/pgspider/pgspider_ext.

Features - Install - Usage yet structured.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 20, 2023

pgspider/griddb_fdw#37 done

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 20, 2023

pgspider/dynamodb_fdw#5 done

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 20, 2023

pgspider/influxdb_fdw#35 done. All PR need carefully reading.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 20, 2023

EnterpriseDB/mysql_fdw#262 done, it seems this repository isn't very active https://github.com/EnterpriseDB/mysql_fdw/pulls.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 21, 2023

CartoDB/odbc_fdw#139 done

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 21, 2023

adjust/parquet_fdw#64 done

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 21, 2023

EnterpriseDB/mongo_fdw#165 done. All PRs has been made. https://github.com/laurenz/oracle_fdw have got well-structured documentation now. I think no need refactoring just now, but it will be reason if all other PRs will merged, because README from firebird_fdw will be standard de facto.

@t-kataym
Copy link
Contributor

Thank you for creating PRs. I will confirm the PR of sqlite_fdw.

By https://github.com/ibarwick/firebird_fdw/blob/master/README.md example of advanced FDW documentation.
Add about functions, encodings, identifiers, FDW options and types of options, refactor features, notes and limitations.
@mkgrgis mkgrgis reopened this Jan 27, 2023
Copy link
Contributor

@t-kataym t-kataym left a comment

Choose a reason for hiding this comment

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

I confirmed the PR. Could you confirm my comments?

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 30, 2023

I confirmed the PR. Could you confirm my comments?

Please verify if all is OK or not.

Copy link

@son-phamngoc son-phamngoc left a comment

Choose a reason for hiding this comment

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

I have some comments. Please help me to check them.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mkgrgis mkgrgis reopened this Feb 4, 2023
Add about OS permissions
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Feb 5, 2023

@t-kataym, I decided to stop the changes in README.md. This PR is very complicated because I still not work with git very well. Since this time I'll save a new changes in README.md for other PR.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@t-kataym
Copy link
Contributor

t-kataym commented Feb 6, 2023

@mkgrgis Thank you for your update.

I decided to stop the changes in README.md ... Since this time I'll save a new changes in README.md for other PR.
Yes, please modify the file for only a fix based on review comment,
and modify new changes (which is not related to review comment) as another PR.

It is unnecessary to squash commits to one.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Feb 6, 2023

It is unnecessary to squash commits to one.

Look like this sentence is for "Contributing" in README.md ;-)

@mkgrgis mkgrgis requested a review from t-kataym February 6, 2023 08:04
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Feb 7, 2023

All problems are fixed, @t-kataym

@t-kataym
Copy link
Contributor

t-kataym commented Feb 8, 2023

@mkgrgis Thank you so much for your PR. I will merge it.

@t-kataym t-kataym merged commit 8847933 into pgspider:master Feb 8, 2023
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Feb 8, 2023

Thank you for your reviews, @t-kataym !

@mkgrgis mkgrgis deleted the patch-1 branch February 8, 2023 02:58
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Apr 1, 2023

@t-kataym , what about my README patches to dynamodb_fdw and influxdb_fdw ? I can parallelly improve documentation and test changes in other pull requests.

@t-kataym
Copy link
Contributor

t-kataym commented Apr 3, 2023

@mkgrgis , we are sorry for no reply. I ask contact persons.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented May 6, 2023

@t-kataym , still no reaction around of https://github.com/pgspider/influxdb_fdw/pull/37/files
In sqlite_fdw is usefully addition to README.md without reaction too https://github.com/pgspider/sqlite_fdw/pull/70/files
Also I'll resolve conflicts in pgspider/jdbc_fdw#14 and actualise this PR to pgspider original documentation.

@t-kataym
Copy link
Contributor

t-kataym commented May 8, 2023

@t-kataym , still no reaction around of https://github.com/pgspider/influxdb_fdw/pull/37/files
In sqlite_fdw is usefully addition to README.md without reaction too https://github.com/pgspider/sqlite_fdw/pull/70/files

We are so sorry for no response.
For influxdb_fdw, I contacted to and asked the maintainer weiting1lee to response it, and assigned him as Reviewers of the PR.
For sqlite_fdw PR#70, Thank you for letting me know. I missed it and will confirm it.

Also I'll resolve conflicts in pgspider/jdbc_fdw#14 and actualise this PR to pgspider original documentation.

Yes, thank you for your contribution.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented May 13, 2023

@t-kataym , I saw there is no topic tags in pgspider repositories. I think it's bad and owners of repositories can add this tags. This tags can improve thematic github searching for a new contributors and gives some PRs. See good example of tags in https://github.com/ibarwick/firebird_fdw
изображение

@mkgrgis
Copy link
Contributor Author

mkgrgis commented May 13, 2023

@t-kataym , what about auto RPM build during https://github.com/pgspider/sqlite_fdw/pull/39/files ? I think it will be usefully not only for for PGXN https://pgxn.org/tag/fdw/

@t-kataym
Copy link
Contributor

@mkgrgis Thank you for your advice and notification.

We will consider to set topics and confirm the PR#39.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented May 26, 2023

@t-kataym , what is the difference between https://github.com/adjust/parquet_fdw and https://github.com/pgspider/parquet_s3_fdw ? Repository from "adjust" tree look like slow-refreshed, but not deprecated. My README PR comes to adjust/parquet_fdw#64 but not to https://github.com/pgspider/parquet_s3_fdw , is it normal? Should I create PR with README to pgspider version of parquet_fdw?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 31, 2024

@t-kataym , look like https://github.com/pgspider/parquet_s3_fdw is completely independent fork of https://github.com/adjust/parquet_fdw and Ajust GmbH does not support initial FDW very good (see adjust/parquet_fdw#64 ).

Does it means I can write a new PR directly to https://github.com/pgspider/parquet_s3_fdw ? This FDW is only with old README.

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