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

Add support for access original data for advanced process and load csv from string. #25

Closed
akeyliu opened this issue May 25, 2019 · 5 comments

Comments

@akeyliu
Copy link

akeyliu commented May 25, 2019

Thanks for your contribution for rapidcsv, and I'm using it in my project.
There are some new requirement from me for advanced usage for rapidcsv, and I hope it can help on improving rapidcsv.

  1. I suggest to add another function to load csv, currently ,there are 2 function to call ReadCSV, one is the csv filename in string, another is the istream. And there is another usally condition to load CSV from string object, which I think need to add it to rapidcsv for user convinience.

  2. I add 2 new functions to access the original csv data wihch is mData in rapidcsv which can provide more powerful access to original data. and I using the follow code.
    /**

    • @brief Get Cell Data by index.
    • @param pRowIdx zero-based row index.
    • @param pColumnIdx zero-based column index.
    • @returns cell data.
      */

    std::string GetCellData(const size_t pColumnIdx, const size_t pRowIdx) const
    {
    const ssize_t columnIdx = pColumnIdx + (mLabelParams.mRowNameIdx + 1);
    const ssize_t rowIdx = pRowIdx + (mLabelParams.mColumnNameIdx + 1);
    return mData.at(rowIdx).at(columnIdx);
    }

    /**

    • @brief Get Cell Data by name.
    • @param pColumnName column label name.
    • @param pRowName row label name.
    • @returns cell data.
      */
      std::string GetCellData(const std::string& pColumnName, const std::string& pRowName) const
      {
      const ssize_t columnIdx = GetColumnIdx(pColumnName);
      if (columnIdx < 0)
      {
      throw std::out_of_range("column not found: " + pColumnName);
      }

    const ssize_t rowIdx = GetRowIdx(pRowName);
    if (rowIdx < 0)
    {
    throw std::out_of_range("row not found: " + pRowName);
    }

    return GetCellData(columnIdx, rowIdx);
    }

@akeyliu akeyliu closed this as completed May 25, 2019
@akeyliu akeyliu reopened this May 25, 2019
@akeyliu akeyliu changed the title Add support for access original data for advanced process. Add support for access original data for advanced process and load csv from string. May 25, 2019
@d99kris
Copy link
Owner

d99kris commented May 26, 2019

Hi @akeyliu - thanks for your input / suggestions.

  1. I have previously thought about adding such API's as well. For consistency it would need a Document() constructor, as well as Load() and Save() overloads. The problem is that these three API's already accept std::string as a first argument, for file path. If you have a proposal for an API that does not conflict with existing API, feel free to share it, then we can discuss and see whether it can be supported by rapidcsv. Meanwhile an application can easily convert a std::string to a stream to read from it, like this:
  const std::string& csv =
    "-,A,B,C\n"
    "1,3,9,81\n"
    "2,4,16,256\n"
    ;

  std::stringstream sstream(csv);
  rapidcsv::Document doc(sstream);

  std::cout << doc.GetCell<int>("A", "1") << "\n"; // prints 3
  std::cout << doc.GetCell<std::string>("C", "2") << "\n"; // prints 256
  1. Regarding the suggested GetCellData() functions, I believe you can already achieve the same (getting cell data as a std::string) using GetCell<std::string>(), so I don't think there is a need to add these GetCellData() functions. Please let me know if I missed something that the GetCellData() provides, and that GetCell<std::string>() does not.

@d99kris
Copy link
Owner

d99kris commented May 26, 2019

I realise that the support for these use-cases are not currently clearly documented, so at least I could try improve the documentation for that.

@akeyliu
Copy link
Author

akeyliu commented Jun 4, 2019

Thanks for your reply and explain!

  1. I know that it is easy to confuse for using file name and string object as the same parameter in the same construct function. My suggestion:
    Solution 1: Still using single string as the parameter, but update the logic in the construct function as follow: (1) check if it's a file name or not, (2) if yes, process as file name, else try to process as an input string; This is my suggestion.
    Solution 2: Add a macro function to convert string to istringstream like follow:
    #define STREAM_FROM_STRING(strObj) {std::istringstream iss(strObj);return(iss) } and suggest user to call it in the construct function instead of using a new construct function.

  2. I will try the GetCellstd::string() instead of GetCellData() for the usage.

@d99kris
Copy link
Owner

d99kris commented Jun 4, 2019

Hi @akeyliu - thanks for the reply and the suggestions.

  1. Regarding solution 1 I feel there are two main problems; it's not common to have one parameter having two different functionalities so it could be confusing to users of the library, and it doesn't allow users to "try/catch" to see if a CSV file exist (as the name would be treated as data if it didn't exist). Regarding solution 2 I do not wish to introduce any defines, see for example https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros for some rationale why it should be avoided. Instead I plan to document this type of usage in the README. I have opened a separate issue Add more examples to README #27 to track this - thanks for highlighting the need for this type of usage. If you wish you can of course fork this repository and add this type of functionality in your own fork.
  2. Ok sounds good!

I will proceed to close this issue.

@d99kris d99kris closed this as completed Jun 4, 2019
@d99kris
Copy link
Owner

d99kris commented Jun 9, 2019

Hi @akeyliu - thanks for highlighting the need of constructing a rapidcsv::Document from a std::string with CSV data. In #29 the documentation was updated to (hopefully) clearly document this use-case. Please let me know if you have any suggestions for improvements. Thanks!

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

No branches or pull requests

2 participants