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

gh-113317: Add libclinic.block_parser and libclinic.language modules #116819

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 14, 2024

  • Move Block and BlockParser classes to a new libclinic.block_parser module.
  • Move Language and PythonLanguage classes to a new libclinic.language module.

* Move Block and BlockParser classes to a new libclinic.block_parser
  module.
* Move Language and PythonLanguage classes to a new
  libclinic.language module.
@vstinner vstinner enabled auto-merge (squash) March 14, 2024 15:26
@vstinner vstinner merged commit b54d7c8 into python:main Mar 14, 2024
38 checks passed
@vstinner vstinner deleted the clinic_block_parser branch March 14, 2024 16:11
@erlend-aasland erlend-aasland changed the title gh-113317, AC: Add libclinic.block_parser module gh-113317: Add libclinic.block_parser and libclinic.language modules Mar 19, 2024
@erlend-aasland
Copy link
Contributor

Would it not be better to create a libclinic.parser and put all parser-related stuff there? Do we need the libclinic.block_parser granularity?

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Mar 19, 2024

Instead of libclinic.language, would it not be better to split things up in the kind of language? For example:

  • libclinic.lang_c for C language render data and the CLanguage class
  • libclinic.lang_python for the PythonLanguage class and its helpers

Now, we suddenly have C language stuff scattered over two submodules: libclinic.language and libclinic.crenderdata.

@erlend-aasland
Copy link
Contributor

Should Block and BlockParser belong to the same submodule? What about BlockPrinter? What was the rationale of putting Block in the libclinic.block_parser submodule?

@vstinner
Copy link
Member Author

What was the rationale of putting Block in the libclinic.block_parser submodule?

I started with very small files to break inter-dependencies, otherwise it's not possible to move any line of code.

Once more more will live in libclinic, it will be easier to move related code together.

CLanguage should live in its on own file, right.

@vstinner
Copy link
Member Author

Should Block and BlockParser belong to the same submodule?

BlockParser creates Block, so they are related, no?

What about BlockPrinter?

I didn't manage to move it yet. But I would prefer to separate parsing code and code genereration code in different files, if possible.

@erlend-aasland
Copy link
Contributor

[...] I would prefer to separate parsing code and code genereration code in different files, if possible.

Yes, this is something we've been discussing. How about putting the intermediate representation (Block) in its own file? After all, it is shared between the parser and the generator. This is why we should discuss changes before merging.

vstinner added a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
* Move Block and BlockParser classes to a new libclinic.block_parser
  module.
* Move Language and PythonLanguage classes to a new
  libclinic.language module.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
* Move Block and BlockParser classes to a new libclinic.block_parser
  module.
* Move Language and PythonLanguage classes to a new
  libclinic.language module.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
* Move Block and BlockParser classes to a new libclinic.block_parser
  module.
* Move Language and PythonLanguage classes to a new
  libclinic.language module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants