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

Bugfix time-Fields retrieve method #9

Closed
germanysources opened this issue Jan 17, 2018 · 5 comments
Closed

Bugfix time-Fields retrieve method #9

germanysources opened this issue Jan 17, 2018 · 5 comments

Comments

@germanysources
Copy link
Contributor

Hallo,

i implemented the following bugfixes:

  1. upload a time field

  2. the retrieve cannot correctly throw the exception retrieve_error (when the method _retrieve throws the lcx_error exception). sy-msgty is undefined. So the runtime error MESSAGE_TYPE_UNKNOWN occurs instead of the exception retrieve_error.

can i upload my new version to this repository?

@germanysources
Copy link
Contributor Author

I added support for abap types int1, int2.
When the mockup loader parse a field, a exception can be thrown, if the component type
is not supported.

@sbcgua
Copy link
Owner

sbcgua commented Aug 26, 2018

Hi @germanysources

Sorry for so late response :)) I hadn't been visiting repo for a long time and missed the notification for some reason.

Yes of course it would be great to merge some new features. However I recently made a lot of code changes and, in particular, split code into several classes and even packages (tab-delimited text parser is now a dependency). So if you have time to adjust the code to the new version it would be cool. Or I can take major features and integrate them and refer you in contributor - how you prefer.

Some comments on the changes:

  • stvarv - cool idea. in terms of code location I'd keep it in mockup loader.
  • I_CHECK_TYPE - not sure I understood the idea
  • LOAD_AND_STORE_TYPE good idea, but I'd unify it in one method (LOAD_AND_STORE). either with another param or dynamic detection.
  • cast ... this is a 7.4 feature afaik. I'd keep compatibility with at least 7.31.
  • CHECK_FOR_RANGE_TYPE - not sure I've got the idea about what does it solve
  • comments - good idea. This should be probably placed in data_parser package. And I'd start comment on the first symbol of the string - this is more reliable.
  • integer, time parsing - sure, good idea. data_parser would be a proper place.
  • float parsing is implemented in data_parser, should be quite flexible, supports number formatting.
  • ZFAST_ASSERTION also a great idea as the macros are quite typical. But I'd rename the include to keep the namespacing proper.

@sbcgua
Copy link
Owner

sbcgua commented Aug 31, 2018

  • implemented I_TYPE_DESC param in LOAD_AND_STORE
  • added integer and time parsing

Though again about stvarv and ZFAST_ASSERTION. Actually I'm not sure that stvarv is the right way - the format settings should be rather project than system specific.

Reviewed my own code for macros and they always specify part of path to mocks so that there is no need to point at folder in the code. e.g. CLASS_XXX/test1_mock - CLASS_XXX is hidden in my macro. The disadvantage is that macro must be defined each time from scratch in each test class. However, it is more convenient to use and read the code after and less error prone ... whereas ZFAST_ASSERTION macros is just the halfway there. Not sure which is better. I'd personally choose readability of the final code (meaning 'specific' macros).

@germanysources
Copy link
Contributor Author

Hello,

my answers:

  1. I_CHECK_TYPE: just added this parameter that the code is compatible. As i seen in the newest version, this parameter is not necessary anymore.

  2. CHECK_FOR_RANGE_TYPE only ranges build from a character field were recognized as a range type in the prior version 1.2.0. Range types of numeric fields weren't recognized in method build_filter.

  3. The ZFAST_ASSERTION include we can keep out of the repository. A gist would be a better place for such small helper scripts.

  4. stvarv should be used as default format settings, if they aren't defined in the project. If you only want to define them in the project, this feature isn't necessary.
    I look if i add can this feature to major version 2.

@sbcgua
Copy link
Owner

sbcgua commented Feb 3, 2019

anything left here ? if not. let's close pls :)

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