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

Detect diamond #77

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open

Conversation

joortcom
Copy link

@joortcom joortcom commented Mar 8, 2024

This problem is more serious than we have thought:

#70 (comment)

Implemented step-wise field path:

#70 (comment)

@ebezault
Copy link
Collaborator

ebezault commented Mar 9, 2024

I cannot accept this pull request as it is. Too many issues to report them one by one in the PR. In summary:

  • The description of the class mentions implicit conversions, not attribute renaming in a diamond.
  • The class contains a lot of unused code copy/pasted from the implicit converts format.
  • The class contains commented out code.
  • New features are missing head comments and assertions.
  • The Gobo project does not use Makefiles but is relying on geant.
  • The logic provided in the Python file should be implemented in Eiffel in class GEDOC_FIELD_RENAME_FORMAT.
  • Use Eiffel terminology, e.g. in Eiffel we don't talk about fields but attributes.
  • The rename test should use getest. It should go into an example folder. Its code does not follow the Eiffel standards (bad formatting, is keyword is deprecated, the use of () violates the uniform access principle, ...).

@mw66
Copy link

mw66 commented Mar 9, 2024

Thanks for looking into this PR. Sorry, I forget to mention that I just want to get your first review opinion (I created the PR rather late in the night at my time, and I was tired and sleepy).

I will fix the concerns you had.

Actually, do you have a better name for the proposed FIELD_RENAME flag?

Another thing I want to discuss beforehand is: do your really think the use of Python is a concern?

The CSV output is easier for the programmers to inspect, and Python has good support for CSV files and file system traversal (i.e

  ecf_fns = glob.glob('**/*.ecf', recursive=True)

)

I'm not sure how easily these things can be done in Eiffel.

(I want the tool to work in small steps, so each step can be inspected and debugged; instead of just one pass, which complicate the development process for novice like me.)

As you can easily tell, my Eiffel programming skill is very limited: from the elementary syntax, the Eiffel library api, and to the GOBO Eiffel compiler's inner working. I only wrote some simple Eiffel code ~20 years ago.

With all these developer constraints, would you consider take the Python script?

Or, would you like to take my implementation just as the new detector's prototype, and start from scratch and do everything in the Eiffel way? I'd be happy if you can help, or just takeover what I have done from here, and make it a proper tool for the Eiffel users as soon as we can, since we know it's a real problem in Eiffel's attribute renaming semantics in real code.

What do you think?

@ebezault
Copy link
Collaborator

ebezault commented Mar 9, 2024

Another thing I want to discuss beforehand is: do your really think the use of Python is a concern?

During the past almost 30 years, I received many contributions like yours. The "problem" with contributions is that when it comes time to maintenance the contributors are not there anymore. At some point I spent more time maintaining the contributions from others than working on my own code. I'm not paid for that, so when that happens I prefer to work with a language that I like and which is the main reason why I started the Gobo project in the first place.

So yes, Python is a concern, and Makefile is a concern.

Or, would you like to take my implementation just as the new detector's prototype, and start from scratch and do everything in the Eiffel way? I'd be happy if you can help, or just takeover what I have done from here, and make it a proper tool for the Eiffel users

I can take over, but I won't have time to work on it before June or July.

as soon as we can, since we know it's a real problem in Eiffel's attribute renaming semantics in real code.

With that respect, I agree with Bertrand Meyer: if only a very small number of the Eiffel programs suffer from this problem, then the priority might be to focus on other problems or missing features that are more important for a larger number of Eiffel users.

@mw66
Copy link

mw66 commented Mar 9, 2024

So yes, Python is a concern, and Makefile is a concern.

No problem, fully understand.

I can take over, but I won't have time to work on it before June or July.

Thanks for taking over. Appreciated.

@mw66
Copy link

mw66 commented Mar 9, 2024

BTW, I'm not paid for any of these effort either. We all are just looking for a better / perfect programming language, and Eiffel is coming close, but didn't make it, esp. in terms of user adoption.

@mw66
Copy link

mw66 commented Mar 10, 2024

is keyword is deprecated

This is to make the code can be tested with the last ECMA compliant SmartEiffel compiler.

@ebezault
Copy link
Collaborator

is keyword is deprecated

This is to make the code can be tested with the last ECMA compliant SmartEiffel compiler.

I don't think we should care about a compiler version which is almost 20 years old.

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.

4 participants