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

Read suffix or prefix from file #12

Conversation

S-Koell
Copy link
Contributor

@S-Koell S-Koell commented Feb 11, 2020

As discussed in #10 :
This adds the functionality to read pre- and suffixes from a .properties file. If none profided it uses the default values.

Open questions from my side (please look at the code first):

  • I am thinking about moving all the variables (Link to code) into a map and loop over the map instead of hard coding the assignment. Do you have a prefered way?
  • The call to the "readProperties" is in a newly created default constructor which is better suited than the getGuidelines method in my view. If you have a better place to move it please tell me beacuse it is still called several times (in the constructor) when doing the tvdcc check.
  • At the moment I only do a println for the "error" case when the properties file doesnt exist. Do you want sth different?
  • I have not written a test case yet. Do you need a testcase to check the "readProperties"?

First pull request ever for me. So if I am doing stuff wrong just let me know :)

@PhilippSalvisberg
Copy link
Collaborator

Thank you for the PR. Actually we do not have documented how to provide a PR. Hence we are going to handle it ad-hoc style.

First I suggest to define a dedicated issue and describe there the change. The change should ensure that the validator works as today, even if no property file is defined. You handled that, that's great, but I would silently ignore the nonexistence of the property file, I mean I would not print a message. Furthermore the validator must work from the CLI and from SQL Developer. I'm not sure that using a hardcoded relative path plugin/custom_variable_prefixes.properties works in all environments. I'd really like to see at least a test case with an existing property file and one without a property file. I think that storing the property file in the user home directory could make it easier (e.g it would run in the test environment as well). This should be documented in the issue to be created. In the end you can commit the changes to the existing branch and this PR will be updated automatically and I can accept and merge it.

@S-Koell
Copy link
Contributor Author

S-Koell commented Feb 18, 2020

All the changes are now in. Please review again :)

(Also closes issue #13 when accepted)

@PhilippSalvisberg PhilippSalvisberg merged commit b128dbb into Trivadis:master Feb 20, 2020
@S-Koell S-Koell deleted the Read_Suffix_or_Prefix_from_File branch February 21, 2020 07:47
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.

None yet

3 participants